c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumCoreBranchRouter.sol#L56-L57

Vulnerability details

Impact

If a token doesn't use string for its name or symbol the adding of such token to the protocol will not work.

One such token is for example MKR token.

Proof of Concept

The following lines where we would like to add a local are affected:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumCoreBranchRouter.sol#L56-L57

Add this to ArbitrumBranchTest.t.sol:

at the begining of the file put this:

import {ERC20} from "solmate/tokens/ERC20.sol";
contract BytesToken {
    bytes32 public constant name = "Token";
    bytes32 public constant symbol = "TKN";
    uint8   public constant decimals = 18;
}

function testTokenBytesName() public {
    BytesToken tkn = new BytesToken();
    string.concat("Arbitrum Ulysses ", ERC20(address(tkn)).name());
}

When running the test will get a revert becouse it's not possible to directly convert string to bytes32.

Running 1 test for test/ulysses-omnichain/ArbitrumBranchTest.t.sol:ArbitrumBranchTest
[FAIL. Reason: EvmError: Revert] testTokenBytesName() (gas: 70565)
Traces:
  [70565] ArbitrumBranchTest::testTokenBytesName()
    ├─ [37687] → new BytesToken@0x74a2eF18ad1352EE820271418F1B11Aae36D6Ab4
    │   └─ ← 188 bytes of code
    ├─ [168] BytesToken::name() [staticcall]
    │   └─ ← 0x546f6b656e000000000000000000000000000000000000000000000000000000
    └─ ← "EvmError: Revert"

Tools Used

Manual review

Recommended Mitigation Steps

Instead of direct calling name() use staticcall() and if the call is not successful try with bytes32 version. If the bytes32 version succeeds then convert the name/symbol to string with a function like this:

function bytes32ToString(bytes32 data) internal pure returns (string memory) {
    bytes memory bytesString = new bytes(32);
    for (uint256 i = 0; i < 32; i++) {
        bytesString[i] = data[i];
    }
    return string(bytesString);
}

Instead of calling ERC(token).name() you can use this (for symbol() is almost the same):

function getTokenName(address tokenAddress) public returns (string memory) {

    (bool success, bytes memory data) = tokenAddress.staticcall(abi.encodeWithSignature("string name()"));
    if (success) {
        return abi.decode(data, (string));
    } else {
        // If the string call fails, try to get the name as a bytes32
        (success, data) = tokenAddress.staticcall(abi.encodeWithSignature("name()"));
        require(success, "Failed to retrieve name");

        bytes32 nameBytes32 = abi.decode(data, (bytes32));

        // Convert bytes32 to string
        bytes memory bytesString = new bytes(32);
        for (uint256 i = 0; i < 32; i++) {
            bytesString[i] = nameBytes32[i];
        }
        return string(bytesString);
    }

}

Assessed type

Error

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #649

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)