Description

Two tokens are compared with the isSameToken function, which calls the calcTokenHash function on the Token structure:

function calcTokenHash(Token memory token) internal pure returns (bytes32) {
    return keccak256(abi.encodePacked(token.chain, token.symbol, token.addr, token.decimals));
}

However, the Token structure is encoded with the function abi.encodePacked. As seen in Finding ref↗, the structure contains dynamic typed fields. Thus, the encoding is not unique and allows collisions in the hash function. Here is a test demonstrating the issue:

function test_HashCollision() public {
    Token memory token1 = Token({
        chain: "BTC",
        symbol: "2200",
        addr: vm.toString(address(WBTC)),
        decimals: WBTC.decimals(),
        amount: 0
    });

    Token memory token2 = Token({
        chain: "BTC2",
        symbol: "200",
        addr: vm.toString(address(WBTC)),
        decimals: WBTC.decimals(),
        amount: 0
    });

    assertTrue(Utils.isSameToken(token1, token2));
}

Impact

The issue seems difficult to exploit since the address often converts with the stringToAddress function, which enforces the address length. Nevertheless, this could be a source of errors and confusion.

Recommendations

The internal function abi.encode should be used instead of abi.encodePacked. More generally, depending on the external usage, only the address could be compared since two tokens that share the same address can be considered as identical.

Remediation