search

sherlock-audit / 2024-05-pooltogether-judging

10 stars 6 forks source link

KupiaSec - The depositors of the vault with tokens with more than 18 decimals like `YAMv2` are unable to withdraw their shutdown balance due to an overflow error despite of large contribution of the vault to the `PrizePool` #144

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 4 months ago

KupiaSec

medium

The depositors of the vault with tokens with more than 18 decimals like YAMv2 are unable to withdraw their shutdown balance due to an overflow error despite of large contribution of the vault to the PrizePool

Summary

When the PrizePool is shutdowned, users can withdraw their shutdown balance by calling the withdrawShutdownBalance() function. It calculates the shutdown balance of the user with contribution of the vault to the PrizePool during grandPrizePeriodDraws days, time weighted balance of the user during grandPrizePeriodDraws days and remained balance of PrizePool. However, users who deposited tokens with more than 18 decimals like YAMv2 into the vaults are unable to withdraw their shutdown balance due to an overflow error in the shutdownBalanceOf function despite of large contribution of the vault to the PrizePool.

Vulnerability Detail

When the PrizePool is shutdown, users can withdraw their shutdown balance by calling the withdrawShutdownBalance() function. The shutdown balance of the user is calculated from the shutdownBalanceOf() function at L942.

    File: pt-v5-prize-pool\src\PrizePool.sol
    938:   function withdrawShutdownBalance(address _vault, address _recipient) external returns (uint256) {
    939:     if (!isShutdown()) {
    940:       revert PrizePoolNotShutdown();
    941:     }
@>  942:     uint256 balance = shutdownBalanceOf(_vault, msg.sender);
    943:     _withdrawalObservations[_vault][msg.sender] = _totalAccumulator.newestObservation();
    944:     if (balance > 0) {
    945:       prizeToken.safeTransfer(_recipient, balance);
    946:       _totalWithdrawn += uint128(balance);
    947:     }
    948:     return balance;
    949:   }

It first calculates the shutdown portion of the user at L916 and available balance of the PrizePool at L929. Then it finally calculates shutdown balance of the user at L931

    File: pt-v5-prize-pool\src\PrizePool.sol
    904:   function shutdownBalanceOf(address _vault, address _account) public returns (uint256) {
            [...]
    913:     // if we haven't withdrawn yet, add the portion of the shutdown balance
    914:     if ((withdrawalObservation.available + withdrawalObservation.disbursed) == 0) {
    915:       (balance, withdrawalObservation) = getShutdownInfo();
@>  916:       shutdownPortion = computeShutdownPortion(_vault, _account); 
    917:       _shutdownPortions[_vault][_account] = shutdownPortion;
    918:     } else {
    919:       shutdownPortion = _shutdownPortions[_vault][_account];
    920:     }
    921: 
    922:     if (shutdownPortion.denominator == 0) {
    923:       return 0;
    924:     }
    925: 
    926:     // if there are new rewards
    927:     // current "draw id to award" observation - last withdraw observation
    928:     Observation memory newestObs = _totalAccumulator.newestObservation();
    929:     balance += (newestObs.available + newestObs.disbursed) - (withdrawalObservation.available + withdrawalObservation.disbursed);
    930: 
@>  931:     return (shutdownPortion.numerator * balance) / shutdownPortion.denominator; 
    932:   }

The shutdown portion of the user is calculated from contribution of the vault to the PrizePool and the time weighted average balance of the user during grandPrizePeriodDraws days which is usually 365 days.

    File: pt-v5-prize-pool\src\PrizePool.sol
    873:   function computeShutdownPortion(address _vault, address _account) public view returns (ShutdownPortion memory) {
    874:     uint24 drawIdPriorToShutdown = getShutdownDrawId() - 1;
    875:     uint24 startDrawIdInclusive = computeRangeStartDrawIdInclusive(drawIdPriorToShutdown, grandPrizePeriodDraws); // @audit-info grandPrizePeriodDraws is usually 365 days
    876: 
@>  877:     (uint256 vaultContrib, uint256 totalContrib) = _getVaultShares( // @audit-info decimal of vaultContrib is 18 
    878:       _vault,
    879:       startDrawIdInclusive,
    880:       drawIdPriorToShutdown
    881:     );
    882: 
@>  883:     (uint256 _userTwab, uint256 _vaultTwabTotalSupply) = getVaultUserBalanceAndTotalSupplyTwab( // @audit-info decimal of vaultContrib is greater than 18 (24 for YAMv2)
    884:       _vault,
    885:       _account,
    886:       startDrawIdInclusive,
    887:       drawIdPriorToShutdown
    888:     );
    889: 
    890:     if (_vaultTwabTotalSupply == 0) {
    891:       return ShutdownPortion(0, 0);
    892:     }
    893: 
@>  894:     return ShutdownPortion(vaultContrib * _userTwab, totalContrib * _vaultTwabTotalSupply);
    895:   }

The shutdown balance of of the user is calculate from L931.

    931:     return (shutdownPortion.numerator * balance) / shutdownPortion.denominator;

shutdownPortion.numerator is multiplication of vaultContrib and _userTwab. So the shutdown balance becomes as following.

             return (vaultContrib * _userTwab * balance) / (totalContrib * _vaultTwabTotalSupply);

Here, the decimal of vaultContrib is 18. As the variable vaultContrib is the amount of contribution to the PrizePool during grandPrizePeriodDraws days which is usually 365 days. If the vault contributed a lot to the PrizePool, it can be large value. Furthermore this value can be increased by calling the contributePrizeTokens() function.

The variable _userTwab represents time weighted average balance of the user during grandPrizePeriodDraws days. Its decimal is greater than 18 (24 for YAMv2).

The variable balance is the available amount of prizetoken in the PrizePool that is entitiled to be withdrawn in case of shutdown which is also large value.

As a result, the multiply of three variables overflows and the shutdownBalanceOf() function is reverted.

Impact

The depositors of the vault with tokens with more than 18 decimals like YAMv2 are unable to withdraw their shutdown balance.

Tool used

Manual Review

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-prize-pool/src/PrizePool.sol#L938-L949

Recommendation

Vaults that uses tokens with more than 18 decimals should be accounted carefully.

sherlock-admin4 commented 3 months ago

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

Low severity__ high decimal tokens are really rare and not used (YAMv2 is the only one on mainnet and 57k$ marketcap)

nevillehuang commented 3 months ago

Duplicate of #137, Invalid per sherlock rules and discussions with HOJ, extremely high decimal tokens was not explicitly highlighted.

  1. Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.