Unsafe cast

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync//blob/main/contracts/AccountCodeStorage.sol#L74-L123

Vulnerability details

Impact

In AccountCodeStorage.sol we have function getCodeHash() and getCodeSize Due to an insecure cast, it is possible to get an integer overflow. Solidity version >0.8.0 provide SafeMath, but casting operations are not safe and can overflow.

Proof of Concept

As can be seen from the code snippet below, getCodeHash intake _input which is uint256. This input is used on line 77 and 78 and as seen below we have unsafe cast from uint160 to uint256.

    /// @notice Simulate the behavior of the `extcodehash` EVM opcode.
    /// @param _input The 256-bit account address.
    /// @return codeHash - hash of the bytecode according to the EIP-1052 specification.
    function getCodeHash(uint256 _input) external view override returns (bytes32) {
        // We consider the account bytecode hash of the last 20 bytes of the input, because
        // according to the spec "If EXTCODEHASH of A is X, then EXTCODEHASH of A + 2**160 is X".

77:        address account = address(uint160(_input));
78:        if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS) {
            return EMPTY_STRING_KECCAK;
        }

        bytes32 codeHash = getRawCodeHash(account);

        // The code hash is equal to the `keccak256("")` if the account is an EOA with at least one transaction.
        // Otherwise, the account is either deployed smart contract or an empty account,
        // for both cases the code hash is equal to the raw code hash.

        if (codeHash == 0x00 && NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(account) > 0) {
            codeHash = EMPTY_STRING_KECCAK;
        }

        // The contract is still on the constructor, which means it is not deployed yet,
        // so set `keccak256("")` as a code hash. The EVM has the same behavior.
        else if (Utils.isContractConstructing(codeHash)) {
            codeHash = EMPTY_STRING_KECCAK;
        }

        return codeHash;

    }

Absolutely the same applies to the function getCodeSize() in the same contract. You need to make sure that the values fit in the variables you are trying to assign them to when casting variables.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting.

[GalloDaSballo]

All addresses fit in u160, that said am flagging this specific one to see if this hits an edge case

[miladpiri]

This is how the EVM calculate extcodesize/extcodehash/codesize/codehash. The low bits are just ignored. It is intended features, not a bug.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

Agree with the sponsor per this line from the spec:

If EXTCODEHASH of A is X, then EXTCODEHASH of A + 2**160 is X.

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid