Description

In the contracts, a token is represented by the following structure:

struct Token {
    string chain;
    string symbol;
    string addr;
    uint8 decimals;
    uint256 amount;
}

To convert the addr field to the type address, the function stringToAddress is used at many places. It converts the hexadecimal string to a number of type address:

function stringToAddress(string memory str) internal pure returns (address) {
    bytes memory strBytes = bytes(str);
    require(strBytes.length == 42, "Invalid address length");
    bytes memory addrBytes = new bytes(20);

    for (uint i = 0; i < 20; i++) {
        addrBytes[i] = bytes1(hexCharToByte(strBytes[2 + i * 2]) * 16 + hexCharToByte(strBytes[3 + i * 2]));
    }

    return address(uint160(bytes20(addrBytes)));
}

However, the function does not check that the two first characters of the string are "0x". Thus, those characters can have any values, but the address returned by stringToAddress will still be the same. This allows bypassing the check of hasDuplicates,

function hasDuplicates(Token[] memory a) internal pure returns (bool) {
    uint256[] memory b = new uint256[](a.length);
    for (uint i = 0; i < a.length; i++) {
        b[i] = uint256(calcTokenHash(a[i]));
    }
    uint256[] memory c = Arrays.sort(b);
    for (uint i = 0; i < c.length - 1; i++) {
        if (c[i] == c[i+1]) {
            return true;
        }
    }
    return false;
}

since the calcTokenHash function uses the full string in the hash calculation. Using duplicate tokens and just changing those two bytes allows having duplicate tokens in the basket.

This problem is also in the hexadecimal address string itself, since uppercase and lowercase letters would result in the same address value but a different hash.

Impact

The duplicate check is used in the AssetRebalancer contract by the addRebalanceRequest to ensure the token set has no duplicate token. Bypassing this check with the previous behavior would allow to incorrectly update the token's amount, as seen in Finding ref↗.

The calcTokenHash computation also breaks the whitelist checks. A token with a different uppercase or lowercase letter in its address string than an identical whitelisted token will not be accepted by the checkOrderInfo function even if it should.

Recommendations

The addr attribute should be of type address to simplify the comparison. It should be converted to hexadecimal string when needed to be displayed.

Remediation

This issue has been acknowledged by SoSoValue, and a fix was implemented in commit 9f023219↗.