PoolTogether - keccak123's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394

Vulnerability details

Impact

The intent of mintYieldFee is to convert the shares that have been collected to the _yieldFeeRecipient with internal accounting to be minted to the address of the assigned recipient. Instead, anyone can call mintYieldFee and assign their address as recipient, taking the fees intended for _yieldFeeRecipient.

Proof of Concept

mintYieldFee has no access modifier and allows _recipient to be set by the caller. The chosen _recipient will receive shares minted to them, taking value from the Vault.

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394

Tools Used

Manual review

Recommended Mitigation Steps

Remove the _recipient argument from mintYieldFee. Modify the mint action to mint directly to _yieldFeeRecipient which is set by the owner.

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1053 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1068

Vulnerability details

Impact

VaultHooks are a way for for winner to call an external smart contract when they win a prize. Any PoolTogether user can set a hook with setHooks. _claimPrize calls beforeClaimPrize and afterClaimPrize hooks of the VaultHooks for every winner. Because the individual winners have control over the address chosen for their own hooks, they can choose a malicious crafted contract that consumes large amounts of gas or reverts. A hook that disrupts the call will cause the user calling _claimPrize to manually remove the offending winner, and if there are several winners with a problematic hook, the caller of _claimPrize may spend extensive gas and time to call _claimPrize, which can further reduce the incentive to assist the protocol with prize claims in the future.

The winner can selectively set the hook to cause the caller of _claimPrize to waste time and gas, then later the winner can set the hook to a different contract and call _claimPrize for their own address, meaning they will only disrupt protocol operations without losing out on their rewards.

Proof of Concept

Any user can call claimPrizes to claim prizes on behalf of others. An array of winners is a function argument https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L80

During the claiming process, the winner's beforeClaimPrize and beforeClaimPrize hooks will be called at the external contract specified by the winner https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1053 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1068

The external contract can implement these hooks to prevent the for loop from completing, disrupting the caller's task of distributing rewards. These are example hooks that revert from an our of gas error or a revert.

  function beforeClaimPrize(
    address winner,
    uint8 tier,
    uint32 prizeIndex
  ) external override returns (address) {
    uint a;
    while(true) {
      a += 1;
    }
  }

  function afterClaimPrize(
    address winner,
    uint8 tier,
    uint32 prizeIndex,
    uint256 payout,
    address recipient
  ) external override {
    revert dos();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Documentation should clarify a process to be taken in cases where the hook set by the winner disrupts claiming operations. Consider adding a protocol incentive to prevent users from setting disruptive hooks, for example a 10% reduction from the original prize amount for every 24 hours that the winner has a disruptive hook set.Claimers could use a simulation process on a fork to confirm that the _claimPrize process will complete without error, but a user who frontruns the process means this is not foolproof, especially if prizes are released at predictable times.

Assessed type

DoS

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L677

Vulnerability details

Impact

Any ERC4626 vault has an underlying ERC20 representing the assets in the vault. setLiquidationPair is an owner-controlled function that set a max approve to an owner-controlled address. This hands over control of the underlying assets to this external address, which can allow the owner to set their own malicious address and use transferFrom to withdraw all assets from the Vault.

Proof of Concept

The max approve happens on this line

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L677

The liquidationPair_ address is set by the owner and there are no checks on this address besides confirming it is not the zero address. Once approved, a malicious owner can use their own malicious contract to withdraw all assets.

Tools Used

Manual review

Recommended Mitigation Steps

Add some safeguards for depositors so that the Vault owner does not have complete control over the vault assets. This is not a simple fix with the existing value design. The new version of PoolTogether is intended to have greater decentralization, so limiting the owner's abilities to control crucial values is important.

Assessed type

Rug-Pull

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L573-L575

Vulnerability details

Impact

liquidate is the only function where _increaseYieldFeeBalance is called. _increaseYieldFeeBalance increases the value of _yieldFeeTotalSupply. When _liquidationPair calls liquidate, the call can get frontrun by the contract owner calling _setYieldFeePercentage, which changes the percentage of _amountOut that is passed to _increaseYieldFeeBalance and in doing directly changes the value of _yieldFeeTotalSupply.

Proof of Concept

The call to _increaseYieldFeeBalance, which increases the value of _yieldFeeTotalSupply, takes place in liquidate

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L573-L575

But setYieldFeePercentage can be called at any time by the owner, such as by frontrunning, allowing the amount of value accumulated for the _yieldFeeRecipient to be fully controlled by the owner.

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L691-L697

Tools Used

Manual review

Recommended Mitigation Steps

Create a new state variable _lastYieldUpdate that stores the current block.timestamp inside of setYieldFeePercentage. A check can then be introduced in liquidate to revert or take other actions if block.timestamp == _lastYieldUpdate, which will prevent a user from becoming a victim of frontrunning from the contract owner.

Assessed type

MEV

I really liked how there are few components that are concisely written in single contracts. This reduces the complexity of the system and makes it easier to understand the design and identify possible weaknesses. There were some parts of the Vault contract that made me raise an eyebrow. For example, _convertToShares is implemented in one internal function but _convertToAssets is implemented in two internal functions. The way that _currentExchangeRate is used in _convertToShares and _convertToAssets is also a bit confusing, because the units and decimals of this value must be kept in mind everywhere that it is used. The relationship and overall flow with the yield vault was not fully clear to me, as was the intended goal of allowing anyone to be the owner of the vault.

To continue on this last point, will PoolTogether follow a similar model to yearn finance v3, where anyone can create strategies that are part of yearn, but yearn will vet the strategies and only feature vetted strategies on their main UI, with the other strategies found on something like ape.tax? Because this model is what makes sense to me given the existing design and desire for decentralization, but there will be continuous effort vetting new strategies, and I saw no documentation of the intention to do this.

I focused on effort on Vault.sol because this is where most of the funds pass through. I did not look at TwabController.sol with the time I allocated so I cannot comment on it.

Time spent:

8 hours