code-423n4 / 2024-03-pooltogether-findings

5 stars 4 forks source link

Liquidation of `_availableYield` could lead to a temporary DoS due to unclaimed `YieldFees` #268

Closed c4-bot-2 closed 7 months ago

c4-bot-2 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Withdrawal of yield generated by underlying yield vault to prize pool would cause a significant drop in the totalasset of the prize vault which is

 function totalAssets() public view returns (uint256) {
        return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this));

this could potentially affect an internal accounting mechanism of the protocol which is totalasset >= totaldebt which is expected to hold true for a number of funtions in the yield vault to work but more importantly deposits and withdrwals. This DoS stems from an increase in accuredYeidfees which is allocated in same function that transfers tokenOut in prizevault:transferTokensOut

        // Increase yield fee balance:
        if (_yieldFee > 0) {
            yieldFeeBalance += _yieldFee;
        }

This increase in YieldFees will cause an incease in totaldebt of the protocol

  function _totalDebt(uint256 _totalSupply) internal view returns (uint256) {
        return _totalSupply + yieldFeeBalance;
    }

If the amount of yield left in the Yield vault after the Prizevault:transferTokensOut is called is not more than yieldFeeBalance it will cause totaldebt > totaldeposit. leading a temporary DOS and stopping deposits(because max deposit ==0 at totaldebt > totalAsset) as well as affecting other functions. Until either new Yeild accures to offset this or till yieldFeeBalance is claimed by yield fee recipient. The current implementation consistently truncates the protocol's operation.

Proof of Concept

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

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

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

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

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

Tools Used

Manual

Recommended Mitigation Steps

The prizevault:transferTokensOut function should call the claimYieldFeeShares function within it.


        // Increase yield fee balance:
        if (_yieldFee > 0) {
            yieldFeeBalance += _yieldFee;
+    uint256 Shares_of_Yield = convertToShares(yieldFeeBalance)
+   claimYieldFeeShares(Shares_of_Yield)

        }

for this to work though claimYieldFeeShares function has to be made a public function not external and not be called by onlyclaimer.

also a check can be put in place for yeild left in the Yeild vault is more than yieldFeeBalance. The former seem more efficient than the latter to me though

Assessed type

DoS

c4-pre-sort commented 7 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #265

c4-judge commented 7 months ago

hansfriese marked the issue as unsatisfactory: Invalid