c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

USDT token has a protection for the approve race condition. It does not allow to change the spender allowance without first changing it to 0.

approve function in USDT on ethereum: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L205

    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);
    }

When the protocol tries to _transferAndApproveToken (a token such is) USDT without first changing allowance of the _localPortAddress (the spender) to 0 will be reverted.

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

Impact

USDT like tokens will not work with the current implementation and will be reverted.

Proof of Concept

Add this POC to BranchBridgeAgentTest.t.sol


// contract at the beging of the file after import
import {ERC20} from "solmate/tokens/ERC20.sol";
contract USDTLikeToken is ERC20 {
    constructor(
        string memory _name,
        string memory _symbol,
        uint8 _decimals
    ) ERC20(_name, _symbol, _decimals) {}

    function approve(address spender, uint256 amount) public override returns (bool) {
        require(!((amount != 0) && (allowance[msg.sender][spender] != 0)), "APPROVE PROTECTION");

        allowance[msg.sender][spender] = amount;
        emit Approval(msg.sender, spender, amount);

        return true;
    }

    function mint(address to, uint256 value) public virtual {
        _mint(to, value);
    }

    function burn(address from, uint256 value) public virtual {
        _burn(from, value);
    }
}

function testApproveToZeroFirst() public {
    address user = address(0x03);
    address localPort = address(0x911);

    USDTLikeToken usdt = new USDTLikeToken("USDT like", "USDTL", 6);
    //Mint Test tokens.
    usdt.mint(user, 100);

    //Approve spend by router
    vm.startPrank(user);
    usdt.approve(address(localPort), 50);
    usdt.transfer(address(localPort), 50);

    usdt.approve(address(localPort), 10);
    vm.stopPrank();
}

When running the POC will get the reverit like :

Running 1 test for test/ulysses-omnichain/BranchBridgeAgentTest.t.sol:BranchBridgeAgentTest
[FAIL. Reason: APPROVE PROTECTION] testApproveToZeroFirst() (gas: 806108)

Tools Used

Manual review

Recommended Mitigation Steps

To be on the safe side first approve to 0

// Check if the underlying tokens are being spent
if (_deposit > 0) {
    _token.safeTransferFrom(msg.sender, address(this), _deposit);
+   ERC20(_token).approve(_localPortAddress, 0);
    ERC20(_token).approve(_localPortAddress, _deposit);
}