sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

0xSpearmint1 - `yieldFeeBalance` cannot be claimed once `TWAB_SUPPLY_LIMIT` is reached #23

Closed sherlock-admin2 closed 4 months ago

sherlock-admin2 commented 4 months ago

0xSpearmint1

high

yieldFeeBalance cannot be claimed once TWAB_SUPPLY_LIMIT is reached

Summary

yieldFeeBalance cannot be claimed once TWAB_SUPPLY_LIMIT is reached

And an attacker can prevent the yieldFeeRecipient from claiming their yieldFeeBalance

Vulnerability Detail

Every vault has a yieldFeeBalance that accumulates every time a liquidation occurs.

Later the designated yieldFeeRecipient can claim the yieldFeeBalance by calling claimYieldFeeShares()

The problem is that once the TWAB_SUPPLY_LIMIT is reached, calls to claimYieldFeeShares() will revert

The function causing the overflow is TwabLib.increaseBalances, it is deep in the flow of claimYieldFeeShares

claimYieldFeeShares ==> _mint ==> twabController.mint ==> ... ==> _increaseTotalSupplyBalances ==> TwabLib.increaseBalances

function increaseBalances(
    uint32 PERIOD_LENGTH,
    uint32 PERIOD_OFFSET,
    Account storage _account,
    uint96 _amount,
    uint96 _delegateAmount
  )
    internal
    ...
    ...
    accountDetails.balance += _amount;
    ...
    ...
  }

The problem is that accountDetails.balance is a type uint96, therefore when the TWAB_SUPPLY_LIMIT is reached, accountDetails.balance = type(uint96).max

Therefore if accountDetails.balance = type(uint96).max AND _amount > 0 then claimYieldFeeShares() will revert

This problem will occur naturally when vaults fill up

But also an attacker can take advantage of this in the following steps

  1. Identify vaults with tokens like SHIBE or LADYS that are not yet full and have a non zero yieldFeeBalance
  2. deposit into the vault such that TWAB_SUPPLY_LIMIT is reached
  3. At this point the yieldFeeBalance that was accumulated can never be claimed

Impact

yieldFeeBalance cannot be claimed

Attacker has caused the yieldFeeRecipient to have locked funds and essentially lose them

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-twab-controller/src/libraries/TwabLib.sol#L114

Tool used

Manual Review

Recommendation

Redesign claimYieldFeeShares so they can always claim it

Duplicate of #39

0xspearmint1 commented 4 months ago

This is a seperate high severity issue

Root cause: yieldFeeBalance is stored as shares

Impact: yieldFeeBalance cannot be claimed and the attacker has caused the yieldFeeRecipient to have locked funds.

Potential Fix: store yieldFeeBalance as tokens

Here are the relevant details of #19 to compare

Root cause: Once the TWAB_SUPPLY_LIMIT is reached, all liquidation attempts will revert due to the _enforceMintLimit() in transferTokensOut()

Impact: The attacker has effectively stolen the yield from other users. The attacker has reduced the chances of winning for all other users of that vault.

Potential Fix: do not _enforceMintLimit() in transferTokensOut()

0xjuaan commented 4 months ago

Escalate

On behalf of @0xspearmint1

sherlock-admin3 commented 4 months ago

Escalate

On behalf of @0xspearmint1

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 4 months ago

Exact same root cause and precondition of TWAB_SUPPLY_LIMIT being reached. I believe they should remain duplicates.

WangSecurity commented 4 months ago

I agree with the lead judge that this should be considered duplicates having the same root cause as said above by the Lead Judge.

Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 4 months ago

Result: High Duplicate of #19

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

WangSecurity commented 4 months ago

This is now duplicate of #39, check here why I believe medium severity is more appropriate.