PoolTogether - Afriauditor's results

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L611

Vulnerability details

Impact

In scenario where totaldebt > totalasset. such as when where is loss of asset in the underlying yieldvault. The conversion from underlying token to prize vault shares changes from 1:1 with more prize vault share required to be exchanged for underlying token. In the function Implementation

 function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;

 >>>>       if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

        yieldFeeBalance -= _yieldFeeBalance;

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

the function takes in number of shares as parameter and also has a logic causing a revert when if (_shares > _yieldFeeBalance) revert This assumes conversion rate will always be 1:1. The implication here is yield fee recipient will be force to convert just a portion of their Yield fee balance to avoid a revert. leaving behind the rest of the yeild fee balance.

also there is a poor implementation here yieldFeeBalance -= _yieldFeeBalance; causing the yeildfee recipient to loose all balance regardless of the number of shares he decides to mint loosing his entire fees either the tokens are minted or not

Proof of Concept

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L611

Tools Used

manual

Recommended Mitigation Steps

++  function claimYieldFeeShares(uint256 _Yieldfeebalancetowithdraw) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
   +    uint256 Yieldfeeshare = convertToshare(_Yieldfeebalancetowithdraw)
   +     yieldFeeBalance -= _Yieldfeebalancetowithdraw

   +     _mint(msg.sender, Yieldfeeshare);

        emit ClaimYieldFeeShares(msg.sender,  Yieldfeeshare);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L798

Vulnerability details

Impact

While not yet minted, the accrued yieldFeeBalance is an inherent part of PrizeVault's shares(totalsupply), impacting the overall totalsupply. This is because minting the yieldFeeBalance as prizevault shares is inevitable (as long as yield fee recipient desires his fees) and really the only way to withdraw YieldFeeBalance. This poses a potential issue as the _twabSupplyLimit function, vital for calculating maxdeposits and maxamountout in liquidatableBalanceOf, omits consideration of minting yieldFeeBalance by yield fee recipient. In extreme scenarios, this oversight could render withdrawals unfeasible by yield fee recipient upon reaching the maximum total supply because it would mean minting prizevault shares exceeding TWAB limits. Which was what the Prizevault:_twabSupplyLimit was implemented to prevent.

Proof of Concept

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L380

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L636

function maxDeposit(address) public view returns (uint256) {
        uint256 _totalSupply =  _maxDeposit();
        uint256 totalDebt_ = _totalDebt(_totalSupply);
        if (totalAssets() < totalDebt_) return 0;

        // the vault will never mint more than 1 share per asset, so no need to convert supply limit to assets
    >>>    uint256 twabSupplyLimit_ = _twabSupplyLimit(_totalSupply); 
        uint256 _maxDeposit;
        uint256 _latentBalance = _asset.balanceOf(address(this));
        uint256 _maxYieldVaultDeposit = yieldVault.maxDeposit(address(this));
        if (_latentBalance >= _maxYieldVaultDeposit) {
            return 0;
        } else {
            unchecked {
                _maxDeposit = _maxYieldVaultDeposit - _latentBalance;
            }
            // @audit would return invalid number number
            return twabSupplyLimit_ < _maxDeposit ? twabSupplyLimit_ : _maxDeposit;
        }
    }

If this is implemented and a user hypothetically decides to use the twabSupplyLimit_ to make a deposit. It would mean TWAB controller's share supply limit is reached. So even when a yield fee recipient wants to claim Yield fee he would be unable to do so. Similar issue will arise from using maxamountout in liquidatableBalanceOf function in the link above.

Tools Used

Manual

Recommended Mitigation Steps

 function _twabSupplyLimit(uint256 _totalSupply) internal pure returns (uint256) {
        unchecked {
            return type(uint96).max - _totalSupply - YieldFeebalance
            
        }
    }

Assessed type

DoS