Quantstamp recently conducted an audit of PoolTogether, a no-loss lottery built on Ethereum. On June 27, PoolTogether was contacted about a potential vulnerability by @epheph and @MicahZoltu of @Keydonix, which was documented and confirmed by Quantstamp.
The impact of this vulnerability is low, as it only affects the current pool, there will be no loss of funds, and no action is needed by current pool participants. Finally, the monetary impact is around $200. The vulnerability has been identified and will be fixed for future pools.
Quantstamp conducted the PoolTogether contract audits in accordance to the following process:
- On June 3, a team of three auditors were selected and assigned to the audit. Between June 6 and June 7, these auditors independently reviewed the functional specification materials, inspected the source code manually, and referenced both the internal audit process guidelines as well as external sources such as Ethereum Smart Contract Best Practices.
- On June 7, the auditors compared notes and discussed the initial findings.
- Between June 10 and June 12, the auditors deployed the smart contracts to Ganache, ran the provided tests, the test coverage tool (solidity-coverage), and automated analysis tools (Mythril and Securify).
- Between June 10 and 14, the report was written and the findings were clarified, formulated, and discussed. In total, 7 issues were identified: one medium-severity, two low-risk, and four informational, as well as some comments on code documentation and best practices.
- On June 14, the initial report was delivered to the PoolTogether team.
- On June 19, findings were discussed with the PoolTogether team, and an updated version of the code was shared.
- Between June 19 and June 21, the diff of the updated code was checked, and the updated report was produced and delivered to the PoolTogether team. Five of the findings were marked as resolved, two were deemed non-issues, and two additional recommendations were included.
- On June 27, the team was made aware of the potential two-winner vulnerability. It was acknowledged by the developers of the contract and confirmed by the auditors. The background information and details on the vulnerability are provided below.
Variation in Return Value of winnerAddress()
In the Pool.sol contract, the method determining the lottery winner — winnerAddress() — relies on the following expression as the source of entropy for random number generation: uint256(blockhash(lockEndBlock) ^ secret) — namely, the hash of the block when the lottery has ended and the secret provided by the pool owner at the pool completion time.
While auditing the code, auditors noticed the return value of the winnerAddress()method may vary depending on the pool state. If the pool is in the UNLOCKED state, the secret is not yet defined, and therefore, entropy value is calculated without taking secret into account. The method winnerAddress() returns a winning address, W0. Once the pool becomes COMPLETE, the secret is defined, and the entropy value then changes. Consequently, the method winnerAddress() now returns an address W1 that is not necessarily the same as W0. The winner W0, however, cannot withdraw winnings because the condition if (state == State.COMPLETE && _addr == winnerAddress())requires the pool to be in the COMPLETE state. So, although winnerAddress()returns two different addresses, there is no financial impact.
As a good practice, the auditors recommended disabling calling winnerAddress() method unless the pool is complete or storing the winner address at the pool completion time and returning the pre-computed variable when winnerAddress() is called (this is on page 7 of the pool together audit report).
During the audit, there was a potential vulnerability that wasn’t identified by the Quantstamp team. 256 blocks after the pool becomes complete, blockhash(lockEndBlock)begins returning zero. As documented, blockhash only works for 256 most recent, excluding current, blocks. This behaviour is counter-intuitive so auditors assumed the function would behave consistently regardless of the invocation time, similar to another built-in function keccak256(). As a result, the entropy changes its value, and winnerAddress() returns a different winner, W2.
With no additional checks on the pool status and other conditions, both winners — W1 and W2 — are eligible to claim the prize: W1 can only claim the prize within the first 256 blocks of the pool completion, while W2 can only claim the prize after this period.
One possible fix is storing the winner address at the pool completion time and returning the pre-computed variable when winnerAddress() is called.
The current pool will continue to run as planned. Upon completion, if the first winner does not claim the prize within 256 blocks they will be manually sent the prize money. If the first and second winner both claim the prize then the pool contract will be funded with a sufficient amount of Dai and all participants can successfully withdraw their deposit. The overall organizational impact is regarded as low, because the issue is scoped to the current pool, and once the issue is fixed, the new pools would implement the logic correctly.
- As part of our continuous improvement process, our auditing process has been extended for future audits to create a checklist for the functions and behaviours that are counterintuitive compared to regular programming languages — this includes the blockhash function.
- We will be improving our new vulnerability mitigation process — this means creating a checklist for responding to discoveries, including immediate proactive support, which we have done in this case. But also making a list of items to consider for an effective PR response — including acknowledging the vulnerability finder, describing the vulnerability and remediation steps, as well as letting the community know whether they need to take any immediate action. Here — we were quite proactive, however having a pre-built list and a rehearsed process will expedite things.
- Bounty reward — we plan to set aside a budget for rewarding the person who discovered the vulnerability — we will use this on a case-by-case basis to supplement the project’s own bounty rewards.
- We are doing an audit of our previous audits (yearly internal review) starting July 1st, to identify any other improvements in our auditing process. We have weekly meetings with all of our auditors, and if someone has practical suggestions for improvements, we will usually adopt these ideas in the coming audits.
- We are continuously improving automation for audits, so for this specific blockhash vulnerability, we will now program it into our auditVM tool to discover it automatically. This should improve our consistency over time with software guided auditing.