sherlock-audit / 2023-11-convergence-judging

8 stars 8 forks source link

bitsurfer - Did not approve to zero first issue #213

Closed sherlock-admin closed 10 months ago

sherlock-admin commented 10 months ago

bitsurfer

medium

Did not approve to zero first issue

Summary

Some tokens like USDT do not allow approve to be called on a non-zero allowance.

Vulnerability Detail

According to README

Receives rewards from the treasury. The list of ERC20 can vary.

Curve (CRV) Convex (CVX) StakeDao (SDT) Frax-Share (FXS) Prisma (PRISMA) USDC USDT DAI

also

Are there any FEE-ON-TRANSFER tokens interacting with the smart contracts?

We have some interaction with USDC and potentially USDT.

and the last,

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?

USDC & USDT

Clearly, the USDT is expected to be used in Convergence.

As we can check on-chain of USDT contract code

    function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

        // To change the approve amount you first have to reduce the addresses`
        //  allowance to zero by calling `approve(_spender, 0)` if it is not
        //  already 0 to mitigate the race condition described here:
        //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

So, seeing this following snippet, it's a well understand that this approval will be reverted because of the approve should set to zero first

File: SdtUtilities.sol
215:         for (uint256 i; i < _tokenSpenders.length; ) {
216:             IERC20(_tokenSpenders[i].token).approve(_tokenSpenders[i].spender, _tokenSpenders[i].amount);
217:             unchecked {
218:                 ++i;
219:             }
220:         }

Impact

Approval of USDT token will fail

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/utils/SdtUtilities.sol#L216

Tool used

Manual Review

Recommendation

Recommend setting allowance to zero first before setting it to a non zero value.

Duplicate of #35