code-423n4 / 2023-03-zksync-findings

6 stars 1 forks source link

``isContractConstructing()`` might return wrong result #94

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/Utils.sol#L49-L52

Vulnerability details

Impact

Detailed description of the impact of this finding. isContractConstructing() might return wrong result

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

isContractConstructing() is testing the equality of a whole byte not testing just the constructor bit. As a result, it might return the wrong result. In particular, if the constructor bit is set, and some other bits are also set, then isContractConstructing() will return a false negative.

Wrong implementation:

 /// @notice Denotes whether bytecode hash corresponds to a contract that is on constructor or has already been constructed
    function isContractConstructing(bytes32 _bytecodeHash) internal pure returns (bool) {
        return _bytecodeHash[1] == 0x01;
    }

For example, suppose _bytecodeHash[1] == 0x11, isContractConstructing() is supposed to return true, but it will return false instead due to its testing of equality of the whole byte not just the bit.

Tools Used

VScode

Recommended Mitigation Steps

Correction:

 /// @notice Denotes whether bytecode hash corresponds to a contract that is on constructor or has already been constructed
    function isContractConstructing(bytes32 _bytecodeHash) internal pure returns (bool) {
        return _bytecodeHash[1] & 0x01;
    }
miladpiri commented 1 year ago

It is the byte that is responsible for the isConstructing.

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

Suggested code doesn't compile

Additionally, the whole byte is picked _bytecodeHash[1] and checked against 0x01 meaning it must be exactly the 0x01 value, which invalidates the finding

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid