The team of PoolTogether contracted us to conduct a software audit of their developed smart contracts written in Solidity. PoolTogether is a company currently developing a no-loss-lottery platform, where users stake DAI in exchange for no-loss lottery tickets. Staked DAI is internally converted into interest-bearing assets, whose accrued interest represents the prize money. As PoolTogether embraces decentralization and trustless-ness, all of this logic is contained in their public smart contracts, not relying on centralized services.
Their smart contracts have been reviewed by us in the course of this audit.
The following services to be provided were defined:
We gained access to the code via the public GitHub repository via https://github.com/pooltogether/pooltogether-pool-contracts. We based the audit on the master branch’s state on October 13th 2020 (commit hash be7ba46ce0ecd1188ab1434d9729e032ff49f482).
As some of our earlier findings have already been addressed in subsequent commits, we also reviewed the code changes that took place up until the latest commit on October 22nd, 2020 (commit hash 14daf57229c32c9d420d5382ac2d7715bfce71f5).
We conducted a manual code review of all the smart contracts that are contained in the repository. While we didn’t focus on the “test” folder of the repository per se, we also went through these.
The code of these contracts has been written according to the latest standards used within the Ethereum community and best practices of the Solidity community. The naming of variables is logical and comprehensible, which results in the contract being good to understand. Most of the code is well documented within the code itself, the overall architecture and concepts are well explained in the online documentation.
There are tests covering the functionalities of the smart contracts in the repository. During our tests, none of these tests failed. We haven’t analyzed all of the tests, but our spot checks on them showed no problems and a well through-out testing strategy.
While most functions have comments to explain what they are doing and which parameters they receive, some are missing. We didn’t list all of the missing ones, just pointed out obviously wrong ones. As these types of mistakes are not severe at all, we haven’t focused too much on this.
The mathematical part has been implemented very well and is handled through battle-tested libraries from OpenZeppelin. The use of uint128 variables makes sense in the circumstances in which they are used, even if they make things a little bit more complex. We haven’t found any problems with this as well.
On the code level, we found no severe bugs or flaws. An additional double-check with two automated reviewing tools (one of them being the highest-paid version of MythX) did not find any bugs at all.
While we did not find any bugs or flaws per se, we found some minor things that we want to note for the sake of completeness.
The comment starting in line 674 appears to be the same as in line 698, while the first function does indeed not claim the drop tokens, the second one does.
The variable is not used anywhere in the project and is internal. It seems to be redundant.
The comment doesn’t match the function of the contract.
/// @title Interface that allows a user to draw an address using an index
The comment doesn’t match the function of the contract.
Note: This has already been fixed in a prior commit, this it is not flagged as severe anymore.
This function is not ideal as it would allow the executor of the transaction to freely define the amount, as this is not part of the permit (which allows basically infinite amounts to be spent). The team already addressed this issue by introducing a check in line 34:
This only allows the original signer to call this function, ensuring that only the desired amount will be deposited. Although this removes the ability to execute this function on behalf of a user, it is probably the best solution to address this issue.
Part of the audit was also an analysis of the protocol and its logic, together with an analysis of whether this protocol works as intended or contains any logical bugs.
The general functionalities and architecture are well described in the PoolTogether online documentation.
Several different modules are working together and are also depending on each other. We haven’t found any problems when working with our own version (that we ran on a local geth-based testnet).
One of the major problems of a lot of DeFi applications is being highly dependent on external protocols and applications (like only relying on cDAI, etc.). The team of PoolTogether did a great job modularizing their components such that they can switch between different prize pools if they see fit. Even if entire external protocols like Compound would cease to work/exist, PoolTogether can keep on working.
The team of PoolTogether is making use of the typical access and role system, where the contract modules themselves have the right to access certain functions. The onlyOwner tag is only used with necessary controlling functions, we can’t see any way how the PoolTogether team could exploit these. There are no functionalities that directly influence user funds in a possibly negative way that the owners have access to.
Users can interact with a lot of the contracts’ functions through the GasStationNetwork, which can lead to some problems because of the nature of the role of the TrustedForwarder. We haven’t found any problems with the implementation of this.
On top of the regular checks and the analysis that we did, we also used some of our own tools (that are still under development) to facilitate some fuzz-testing and other methods to try getting the contract into undesired and wrong states. We weren’t able to do so in the course of our extensive testing. We also weren’t able to access any user funds through our techniques.
We were not able to spot any further issues in the logic and the protocol of the PoolTogether V3 implementation.
During our code review (which was done manual as well as automated) we did not find any bugs or flaws, just some minor recommendations noted in section 2.1.
In our analysis of the protocol and the logic of the architecture, we found no flaws and we were not able to maliciously game the protocol through the tests that we did.
We are very happy with the overall quality of the code as well as the tests that have been written.
In terms of documentation the code comments could be extended to all of the functions, since some are missing, but overall we are very happy with the current state as well.
As of the date of publication, the information provided in this report reflects the presently held understanding of the auditor’s knowledge of security patterns as they relate to the client’s contract(s), assuming that blockchain technologies, in particular, will continue to undergo frequent and ongoing development and therefore introduce unknown technical risks and flaws. The scope of the audit presented here is limited to the issues identified in the preliminary section and discussed in more detail in subsequent sections. The audit report does not address or provide opinions on any security aspects of the Solidity compiler, the tools used in the development of the contracts or the blockchain technologies themselves, or any issues not specifically addressed in this audit report.
The audit report makes no statements or warranties about the utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, the legal framework for the business model, or any other statements about the suitability of the contracts for a particular purpose, or their bug-free status.
To the full extent permissible by applicable law, the auditors disclaim all warranties, express or implied. The information in this report is provided “as is” without warranty, representation, or guarantee of any kind, including the accuracy of the information provided. The auditors hereby disclaim, and each client or user of this audit report hereby waives, releases and holds all auditors harmless from, any and all liability, damage, expense, or harm (actual, threatened, or claimed) from such use.
We store our public audit reports on IPFS; a peer-to-peer network called the "Inter Planetary File System". This allows us to store our reports in a distributed network instead of just a single server, so even if our website is down, every report is still available.
The IPFS Hash, a unique identifier of the report, is signed on-chain by both the client and us to prove that both sides have approved this audit report. This signing mechanism allows users to verify that neither side has faked or tampered with the audit.
