PoolTogether V3

November 17, 2020

1. Preface

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:

  • Manual code review
  • Protocol/Logic review and analysis
  • A written summary of all the findings and suggestions on how to remedy them as well as the outcome of the comparison

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).

2. Manual Code Review

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.

2.1. Bugs & Vulnerabilities

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.

A) comptroller/Comptroller.sol Line 674 & 698 [NO SEVERITY]
/// @notice Updates the given drips for a user and then claims the given drip tokens.

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.

B) prize-strategy/PeriodicPrizeStrategy.sol Line 35 [NO SEVERITY]
uint256 internal constant ETHEREUM_BLOCK_TIME_ESTIMATE_MANTISSA;

The variable is not used anywhere in the project and is internal. It seems to be redundant.


C) reserve/Reserve.sol Line 10 [NO SEVERITY]
/// @title Interface that allows a user to draw an address using an index

The comment doesn’t match the function of the contract.


D) reserve/ReserveInterface.sol Line 5 [NO SEVERITY]

/// @title Interface that allows a user to draw an address using an index

The comment doesn’t match the function of the contract.


E) permit/PermitAndDeposit.sol Line 29-37 [NO SEVERITY]

Note: This has already been fixed in a prior commit, this it is not flagged as severe anymore.

function permitAndDepositTo()

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:

require(msg.sender == holder, "PermitAndDepositDai/only-signer");

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.


3. Protocol/Logic Review

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.

4. Summary

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.

Stored on IPFS

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.

Learn more about IPFS
Signed On-Chain

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.

Check the Signatures