carrot - Incorrect handling of token approvals

[github-actions[bot]]

carrot

high

Incorrect handling of token approvals

Summary

The BlueBerryBank contract approves token spending. However, it never resets token approvals 0. This can cause issues with tokens like CRV, where you cannot set approvals from a non-zero value to another non-zero value.

Vulnerability Detail

Approvals are generally handled in two ways. In the normal Openzeppelin pattern, approve function sets the allowance to the value passed. However in other tokens, like USDT, there is a built-in race condition prevention mechanism where approvals must be reset to 0 before setting it to some non-zero value. This is also true for the case of the CRV token, as can be seen from its vyper contract

def approve(_spender : address, _value : uint256) -> bool:
    """
    @notice Approve `_spender` to transfer `_value` tokens on behalf of `msg.sender`
    @dev Approval may only be from zero -> nonzero or from nonzero -> zero in order
        to mitigate the potential race condition described here:
        https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
    @param _spender The address which will spend the funds
    @param _value The amount of tokens to be spent
    @return bool success
    """
    assert _value == 0 or self.allowances[msg.sender][_spender] == 0
    self.allowances[msg.sender][_spender] = _value
    log Approval(msg.sender, _spender, _value)
    return True

Since the protocol is intent on using CRV, it must use precaution with the approval mechanism and have the bank reset the approval to 0, after every instance of it granting approval. Otherwise, it can permanently brick the contract if even 1 wei of approval is remaining after calling the transfer functions, as it will revert on subsequent approval calls. Since the bank sometimes depends on external contracts to move out tokens (lending protocol, spells), the current approval pattern is very risky.

Impact

Bricked bank for CRV tokens due to incorrect approval handling

Code Snippet

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L646-L657 https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L882-L884

Tool used

Manual Review

Recommendation

Explicitly reset approval to 0 after setting it at the end of the functions

IERC20Upgradeable(token).approve(bank.cToken, 0);

[pauliax]

Escalate for 3 USDC:

Essentially the same as #236. Please refer to the comment where I try to explain why I think it should be low: https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/236#issuecomment-1475340133

However, I believe this specific issue (#159) should be invalid altogether because it talks about these 2 cases:

        if (address(ISoftVault(bank.softVault).uToken()) == token) {
            IERC20Upgradeable(token).approve(bank.softVault, amount);
            pos.underlyingVaultShare += ISoftVault(bank.softVault).deposit(
                amount
            );
        } else {
            IERC20Upgradeable(token).approve(bank.hardVault, amount);
            pos.underlyingVaultShare += IHardVault(bank.hardVault).deposit(
                token,
                amount
            );
        }

        IERC20Upgradeable(token).approve(bank.cToken, amountCall);
        if (ICErc20(bank.cToken).repayBorrow(amountCall) != 0)
            revert REPAY_FAILED(amountCall);

Clearly, the .approve() is instantly followed by an action (.deposit(), .repayBorrow()) that drains the exact amount approved. If you investigate all these functions perform safeTransferFrom so approval should never reach the state when it is not 0. Watsons tried to pick low-hanging fruit here by generalizing issue without the proper context.

[sherlock-admin]

Escalate for 3 USDC:

Essentially the same as #236. Please refer to the comment where I try to explain why I think it should be low: https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/236#issuecomment-1475340133

However, I believe this specific issue (#159) should be invalid altogether because it talks about these 2 cases:

        if (address(ISoftVault(bank.softVault).uToken()) == token) {
            IERC20Upgradeable(token).approve(bank.softVault, amount);
            pos.underlyingVaultShare += ISoftVault(bank.softVault).deposit(
                amount
            );
        } else {
            IERC20Upgradeable(token).approve(bank.hardVault, amount);
            pos.underlyingVaultShare += IHardVault(bank.hardVault).deposit(
                token,
                amount
            );
        }

        IERC20Upgradeable(token).approve(bank.cToken, amountCall);
        if (ICErc20(bank.cToken).repayBorrow(amountCall) != 0)
            revert REPAY_FAILED(amountCall);

Clearly, the .approve() is instantly followed by an action (.deposit(), .repayBorrow()) that drains the exact amount approved. If you investigate all these functions perform safeTransferFrom so approval should never reach the state when it is not 0. Watsons tried to pick low-hanging fruit here by generalizing issue without the proper context.

You've created a valid escalation for 3 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[carrotsmuggler]

As stated above, CRV, which also follows a similar pattern as USDT, is in scope. So i dont think the comment on the other report applies for this case.

[hrishibhat]

Escalation accepted

Not a valid medium As pointed out in the escalation approval is followed by an immediate transfer of the exact amount of tokens approved.

[sherlock-admin]

Escalation accepted

Not a valid medium As pointed out in the escalation approval is followed by an immediate transfer of the exact amount of tokens approved.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.