PoolTogether - Inspecktor's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299-L306 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335

Vulnerability details

Impact

The drawManager parameter can be set in the constructor (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278). However, if drawManager = 0 is passed to the constructor, it can be set later in setDrawManager() (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299- L306). The drawManager parameter in setDrawManager() is only updated once.

If you don't set drawManager in the constructor, an attacker can get ahead and set any address in setDrawManager(). The administrator will not be able to change it in the future.

The drawManager parameter is used in modifier onlyDrawManager() { if (msg.sender != drawManager) { revert CallerNotDrawManager(msg.sender, drawManager); } _; } at the time of checking access to the withdrawReserve() and closeDraw() functions.

Suppose someone managed to deposit funds into the prize pool via increaseReserve(). This could result in the funds in withdrawReserve() being stolen by an attacker (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335).

Tools Used

Manual review

Recommended Mitigation Steps

Only set the value of drawManager in the constructor. Or set the onlyOwner check for setDrawManager().

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/VaultFactory.sol#L67-L78

Vulnerability details

Impact

The deployVault function deploys a clone contract using create, where getting the address depends only on the VaultFactory nonce.

Reorganization can occur in all EVM chains. In Ethereum, where pooltogether is currently deployed, this is not "super common" but still happening, being the last one less than a year ago: https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg

An issue can arise when users rely on getting an address beforehand, or try to deploy a clone of a position with the same address on different EVM chains, any funds sent to the new clone could potentially be withdrawn by someone else. In general, this can lead to the theft of users' funds.

An example of a similar error: https://code4rena.com/reports/2023-01-rabbithole#m-01-questfactory-is-suspicious-of-the-reorg-attack

Proof of Concept

Imagine that Alice deploys a clone of a position and then sends funds to it. Bob sees that the network block is being reorganized and calls deployVault. Thus, a clone of the position is created with the address to which Alice sends funds. Alice's transactions are then executed and Alice transfers the funds to Bob's position contract.

Tools Used

Manual review

Recommended Mitigation Steps

Deploy Vault via create2 with salt which includes msg.sender and _asset address.

Assessed type

Context

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L498-L502

Vulnerability details

Impact

Part of the contributions to the vault is recorded as a reserve. This part serves two purposes:

• finances incentives for participation in the draw;
• provides an airbag in case of insufficient liquidity of the level for prizes.

The reserve contribution uses increaseReserve() (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L498-L502), allowing anyone to contribute directly to the prize pool reserve. However, it is not taken into account that some tokens tend to take a built-in commission for the transfer of a token. Thus, the PrizePool.sol account will receive less than expected. At a certain moment, there may not be any tokens in the reserve account at all. Although the contract will assume that there are tokens.

Proof of Concept

https://github.com/d-xo/weird-erc20#fee-on-transfer

Tools Used

Manual review

Recommended Mitigation Steps

In increaseReserve() write the old balance of prizeToken. Transfer the prizeToken to the contract address. Read new balance. Record the difference between the balances of the new and the old in _reserve.

Assessed type

Token-Transfer