search

code-423n4 / 2023-09-maia-findings

25 stars 17 forks source link

USDT tokens should approve to zero first otherwise it may cause other tokens to get stuck in the case of aprooveMultipleTokens #896

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L160-L176

Vulnerability details

Impact

Unsafe ERC20 approve that do not handle non-standard erc20 behavior. Some token contracts do not return any value. Some token contracts revert the transaction when the allowance is not zero.

Proof of Concept

When receiving tokens to the destination chain, the token attempts to approve the destination port address.

        if (_deposit > 0) {
            _token.safeTransferFrom(msg.sender, address(this), _deposit);
            ERC20(_token).approve(_localPortAddress, _deposit);
        }

For a token like USDT, the token needs to be approved to zero first before calling approve. This may cause a problem in the case of multiple token transfers, and because usdt is not approve, the other tokens may be stuck in the transfer process

Since the omnichain uses all sorts of tokens, it is important that the weird ERC20 tokens are counted as well.

Tools Used

VSCode

Recommended Mitigation Steps

It is recommended to set the allowance to zero before increasing the allowance and use safeApprove/safeIncreaseAllowance.

Assessed type

Other

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

0xA5DF commented 1 year ago

Automated findings

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

alcueca commented 1 year ago

https://github.com/code-423n4/2023-09-maia/blob/main/bot-report.md#l15-approvesafeapprove-may-revert-if-the-current-approval-is-not-zero

c4-judge commented 1 year ago

alcueca marked the issue as unsatisfactory: Out of scope