Tokens with incorrect ERC20 implementation for return statements would emit failure events.

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x07d7b12a67b7902c13aaa6acbe201d7bf346d802f7c366a3f816c0e39a026ade Severity: minor

Description: Description\ Some tokens do not return a boolean on ERC methods, most notably USDT and BNB being the most popular tokens to do so and based on provided context, these tokens would be used. More such tokens: https://gist.githubusercontent.com/lukas-berlin/f587086f139df93d22987049f3d8ebd2/raw/1f937dc8eb1d6018da59881cbc633e01c0286fb0/Tokens%20missing%20return%20values%20in%20transfer

The problem is in tokenTransferReturnSuccess, where the return value depends on the success, which would always be false for these tokens as they do not return a value. This would not lead to a revert, but the failure event would still be fired, thus marked Minor.

Attack Scenario\ When the request's token is not WETH, and we initiate a transfer, tokens like the ones above would complete their transfers, but still fire of the failure event, potentially tampering with off-chain monitoring and commitees.

Another aspect of the issue is the fact that tokens which's transfers fail would not cause a revert, leading to them being stuck.

Attachments

1. Proof of Concept (PoC) File

For the PoC, I first modified the Token.sol file in the contracts/ folder and added:

function transfer(address to, uint256 value) public virtual override returns(bool){
        address owner = _msgSender();
        _transfer(owner, to, value);
        return false;
    }

to override the default ERC20 transfer to act like BNB and USDT

The PoC:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19.0;

import {Test, console} from "forge-std/Test.sol";
import "forge-std/console.sol";
import {Most} from "../contracts/Most.sol";
import {Token} from "../contracts/Token.sol";

contract NoReturnTest is Test {
    Most m;
    address user = address(1);
    address receiver = address(2);
    address[] commitee = new address[](1);

    Token mockTokenNoReturn;

    event TokenTransferFailed(bytes32 requestHash);

    function setUp() public {
        commitee[0] = user;
        m = new Most();
        m.initialize(commitee, 1, address(this), payable(0));
        mockTokenNoReturn = new Token(1_000_000, 18, "USDT-Mock", "USDTM");
        mockTokenNoReturn.transfer(user, 10_000);

        m.unpause();
    }

    function test_failForTokensNoReturn() public {
        vm.startPrank(user);

        bytes32 requestHash = keccak256(
            abi.encodePacked(
                uint256(0),
                bytes32(uint256(uint160(address(mockTokenNoReturn)))),
                uint256(1000),
                bytes32(uint256(uint160(address(receiver)))),
                uint256(0)
            )
        );

        vm.expectEmit(true, false, false, false);
        emit TokenTransferFailed(requestHash);

        m.receiveRequest(
            requestHash, 
            0,
            bytes32(uint256(uint160(address(mockTokenNoReturn)))),
            1000,
            bytes32(uint256(uint160(address(receiver)))),
            0
        );
    }
}

Even though the transfer would be successful, we still emit the event for failure. As said above, another aspect is that failing transfer do not lead to a revert of the function, which can be a problem on it's own (which could lean more towards Low)

2. Revised Code File (Optional)

Recommendation: usage of OZ's safe functions instead of low level calls to make sure such ERC20s return their respective values.

P.S This shouldn't be OOS, since the rules specify malicious tokens, USDT is a well-established stable coin planned for compatibility and such behavior of it is known on the mainnet and should be mitigated against.

[krzysztofziobro]

The check implemented in tokenTransferReturnSuccess has practically the same logic as the one used in safeTransfer. There is an issue with the PoC - tokens like USDt do not return the value at all, and your version always returns false, which is handled differently.

[PlamenTSV]

Tokens that do not have a return value return the default value - false and would result in the same effect replicated above. Also tokens that always return false do exist aswell.