sherlock-audit / 2023-10-notional-judging

5 stars 5 forks source link

ZanyBonzy - Logic for revert on max approval tokens #68

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

ZanyBonzy

medium

Logic for revert on max approval tokens

Summary

In the _initialApproveTokens functions, maximum token amount type(uint256).max is approved to the vault, pools and boosters.

Vulnerability Detail

Some ERC20 tokens (e.g. UNI, COMP) revert if the value passed to approve is larger than type(uint96)max. Special conditions have to be made for these token types to prevent unexpected behaviours.

Impact

A number of features within the vaults will not work if the approve function reverts.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L52C1-L61C1

    function _initialApproveTokens() internal override {
        (IERC20[] memory tokens, /* */) = TOKENS();
        for (uint256 i; i < tokens.length; i++) {
            tokens[i].checkApprove(address(Deployments.BALANCER_VAULT), type(uint256).max); //@note
        }

        // Approve Aura to transfer pool tokens for staking
        POOL_TOKEN().checkApprove(address(AURA_BOOSTER), type(uint256).max); //@note
    }

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/curve/ConvexStakingMixin.sol#L53C1-L58C6

    function _initialApproveTokens() internal override {
        // If either token is Deployments.ETH_ADDRESS the check approve will short circuit
        IERC20(TOKEN_1).checkApprove(address(CURVE_POOL), type(uint256).max);
        IERC20(TOKEN_2).checkApprove(address(CURVE_POOL), type(uint256).max);
        CURVE_POOL_TOKEN.checkApprove(address(CONVEX_BOOSTER), type(uint256).max);
    }

Tool used

Manual Review

Recommendation

Make a special case for these token types, or remove them from supported list.

Duplicate of #93

sherlock-admin2 commented 11 months ago

Escalate

Hi,

I think this should be considered because these tokens don't take type(uint256).max as infinite approvals, but downcast it to type(uint96).max and takes this as raw amount, i.e not infinite approval unlike other normal tokens. Consequently, as spending goes on, the approval will eventually reach zero, at which point the calling contracts will no longer function properly.

Thanks for your time.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.