BaseBranchRouter._transferAndApproveToken may revert in some cases

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BaseBranchRouter.sol#L175

Vulnerability details

Impact

In bot-report.md, [M-04] Return values of approve() not checked. It describles that "By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything". But this report is different from [M-04].

If the token's approve does not return a value, like USDT, then ERC20(_token).approve will revert. Because ERC20 comes from solmate/tokens/ERC20.sol, its approve is defined as function approve(address spender, uint256 amount) public virtual returns (bool) where the return value is bool type with a size of 1 byte. The bytecode compiled by the solidity compiler from ERC20(_token).approve will check whether the return size is 1 byte. If not, revert will occur.

This will cause all functions calling _transferAndApproveToken to revert. This includes callOutAndBridge and callOutAndBridgeMultiple.

Proof of Concept

File: src\BaseBranchRouter.sol
160:     function _transferAndApproveToken(address _hToken, address _token, uint256 _amount, uint256 _deposit) internal {
......
173:         if (_deposit > 0) {
174:             _token.safeTransferFrom(msg.sender, address(this), _deposit);
175:->           ERC20(_token).approve(_localPortAddress, _deposit);
176:         }
177:     }

L175, the compiled code of ERC20(_token).approve(_localPortAddress, _deposit) is similar to the following:

(bool ret, bytes data) = _token.call(abi.encodeWithSignature("approve(address,uint256)", _localPortAddress, _deposit);
if (ret) {
     if (data.length != 1) // since usdt.approve has no return value, so data.length = 0
     {
            revert;
     }
     return abi.decode(data, (bool));
} else {
     revert;
}

Copy the coded POC below to one project from Foundry and run forge test -vvv to prove this issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import "forge-std/Test.sol";
import "solmate/tokens/ERC20.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

interface CheatCodes {
    function prank(address) external;
    function createSelectFork(string calldata,uint256) external returns(uint256);
}

interface IUSDT {
    function approve(address, uint256) external;
}

contract ContractTest is DSTest{

    address USDT = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
    CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);

    function setUp() public {
        cheats.createSelectFork("https://rpc.ankr.com/eth", 18283438);
    }

    function test() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        ERC20(USDT).approve(address(0x1111111111), 1000e6);
    }

    function testOkByIUSDT() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        IUSDT(USDT).approve(address(0x1111111111), 1000e6);
    }

    function testOkBySafeApprove() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        SafeTransferLib.safeApprove(ERC20(USDT), address(0x1111111111), 1000e6);
    }
}

/**output
Running 3 tests for src/test/usdtApprove.sol:ContractTest
[FAIL. Reason: EvmError: Revert] test() (gas: 37109)
Traces:
  [37109] ContractTest::test()
    ├─ [0] VM::prank(0x0000000000000000000000000012399543949349)
    │   └─ ← ()
    ├─ [26953] 0xdAC17F958D2ee523a2206206994597C13D831ec7::approve(0x0000000000000000000000000000001111111111, 1000000000)
    │   ├─ emit Approval(owner: 0x0000000000000000000000000012399543949349, spender: 0x0000000000000000000000000000001111111111, value: 1000000000)  
    │   └─ ← ()
    └─ ← "EvmError: Revert"

[PASS] testOkByIUSDT() (gas: 37113)
[PASS] testOkBySafeApprove() (gas: 36988)
Test result: FAILED. 2 passed; 1 failed; finished in 489.70ms
**/

Tools Used

Manual Review

Recommended Mitigation Steps

Use safeApprove from solady/utils/SafeTransferLib.sol.

File: src\BaseBranchRouter.sol
160:     function _transferAndApproveToken(address _hToken, address _token, uint256 _amount, uint256 _deposit) internal {
......
173:         if (_deposit > 0) {
174:             _token.safeTransferFrom(msg.sender, address(this), _deposit);
175:-            ERC20(_token).approve(_localPortAddress, _deposit);
175:+            ERC20(_token).safeApprove(_localPortAddress, _deposit);
176:         }
177:     }

Assessed type

DoS

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #896

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[c4-judge]

alcueca marked the issue as unsatisfactory:
Out of scope

[alcueca]

USDT reverts because it needs to approve to zero first, which is also in the bot report:
https://github.com/code-423n4/2023-09-maia/blob/main/bot-report.md#l15-approvesafeapprove-may-revert-if-the-current-approval-is-not-zero

[securitygrid]

The issue described in this report is not about approving to zero first, but the difference between usdt.approve and ERC20.approve signatures:
usdt.approve is defined as follows:
function approve(address _spender, uint _value) public;
ERC20.approve is defined as follows:
function approve(address spender, uint256 amount) public virtual returns (bool);
This will lead to different opcodes compiled by the compiler: when checking the length of the return data, usdt.approve requires the length of the return data to be 0, while ERC20.approve requires the length of the return data to be 1. Therefore, tx always reverts.
The POC of this report is to prove this issue, not approve to zero first. Please review, thank you.

[c4-judge]

alcueca marked the issue as not a duplicate

[c4-judge]

alcueca marked the issue as satisfactory

[alcueca]

Very good PoC and explanation. Apologies for getting the report confused with #896

[alcueca]

Note that while this deviation from the standard only happens on the mainnet variant of USDT, and not on the Arbitrum one, it is likely that the protocol would be extended with branches to mainnet. Non-adherence to the ERC20 standard in the case of mainnet USDT can't be considered a poisoned token, and therefore the Medium severity is sustained.

[0xBugsy]

Addressed at Maia-DAO/2023-09-maia-remediations@cbdd6e2

[c4-sponsor]

0xBugsy (sponsor) confirmed