sherlock-audit / 2023-07-blueberry-judging

2 stars 1 forks source link

0x52 - WAuraPools doesn't correctly account for AuraStash causing all deposits to be permanently lost #108

Open sherlock-admin2 opened 1 year ago

sherlock-admin2 commented 1 year ago

0x52

high

WAuraPools doesn't correctly account for AuraStash causing all deposits to be permanently lost

Summary

Some Aura pools have two sources of AURA. First from the booster but also as a secondary reward. This secondary reward is stash AURA that doesn't behave like regular AURA. Although properly accounted for in AuraSpell, it is not properly accounted for in WAuraPools, resulting in all deposits being unrecoverable.

Vulnerability Detail

WAuraPools.sol#L413-L418

    uint256 rewardTokensLength = rewardTokens.length;
    for (uint256 i; i != rewardTokensLength; ) {
        IERC20Upgradeable(rewardTokens[i]).safeTransfer(
            msg.sender,
            rewards[i]
        );

When burning the wrapped LP token, it attempts to transfer each token to msg.sender. The problem is that stash AURA cannot be transferred like an regular ERC20 token and any transfers will revert. Since this will be called on every attempted withdraw, all deposits will be permanently unrecoverable.

Impact

All deposits will be permanently unrecoverable

Code Snippet

WAuraPools.sol#L360-L424

Tool used

Manual Review

Recommendation

Check if reward is stash AURA and send regular AURA instead similar to what is done in AuraSpell.

sherlock-admin2 commented 1 year ago

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

Kral01 commented:

Needs PoC

IAm0x52 commented 1 year ago

Escalate

This was incorrectly excluded.

https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/spell/AuraSpell.sol#L243-L257

    for (uint256 i; i != rewardTokensLength; ) {
        address sellToken = rewardTokens[i];
        if (sellToken == STASH_AURA) sellToken = AURA;

        _doCutRewardsFee(sellToken);
        if (
            expectedRewards[i] != 0 &&
            !PSwapLib.swap(
                augustusSwapper,
                tokenTransferProxy,
                sellToken,
                expectedRewards[i],
                swapDatas[i]
            )
        ) revert Errors.SWAP_FAILED(sellToken);

        /// Refund rest (dust) amount to owner
        _doRefund(sellToken);

        unchecked {
            ++i;
        }

AuraSpell requires the above code to prevent this. WAuraPools uses the exact same reward list and needs the same protections. The result is that funds will be permanently trapped, because the transfer will always fail.

sherlock-admin2 commented 1 year ago

Escalate

This was incorrectly excluded.

https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/spell/AuraSpell.sol#L243-L257

    for (uint256 i; i != rewardTokensLength; ) {
        address sellToken = rewardTokens[i];
        if (sellToken == STASH_AURA) sellToken = AURA;

        _doCutRewardsFee(sellToken);
        if (
            expectedRewards[i] != 0 &&
            !PSwapLib.swap(
                augustusSwapper,
                tokenTransferProxy,
                sellToken,
                expectedRewards[i],
                swapDatas[i]
            )
        ) revert Errors.SWAP_FAILED(sellToken);

        /// Refund rest (dust) amount to owner
        _doRefund(sellToken);

        unchecked {
            ++i;
        }

AuraSpell requires the above code to prevent this. WAuraPools uses the exact same reward list and needs the same protections. The result is that funds will be permanently trapped, because the transfer will always fail.

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.

Shogoki commented 1 year ago

Seems valid STASH AURA seems to be a valid extraRewardToken but transfer can only be called from the pool, which will actually transfer AURA instead. So if the WAURAPool tries to call transfer it will revert.

hrishibhat commented 1 year ago

Result: Medium Unique Considering this issue a valid medium

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: