Function getImmutable() should revert for non-existing immutable for an address instead of returning 0

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ImmutableSimulator.sol#L26-L28 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ImmutableSimulator.sol#L33-L43

Vulnerability details

Impact

Contract ImmutableSimulator is a system smart contract that simulates the behavior of immutable variables in Solidity. by default contract's code returns 0 for any undefined immutable index, as there is no way to detect that how many immutable a contract had and there is no way to distinguish 0 value immutable for non-existing immutable so any logic accessing immutable values of and address can be vulnerable if it relies on ImmutableSimulator return values. function getImmutable() should revert for non existing immutables.

Proof of Concept

This is getImmutable() and setImmutables() codes:

    /// @notice Method that returns the immutable with a certain index for a user.
    /// @param _dest The address which the immutable belongs to.
    /// @param _index The index of the immutable.
    /// @return The value of the immutables.
    function getImmutable(address _dest, uint256 _index) external view override returns (bytes32) {
        return immutableDataStorage[uint256(uint160(_dest))][_index];
    }

    /// @notice Method used by the contract deployer to store the immutables for an account
    /// @param _dest The address which to store the immutables for.
    /// @param _immutables The list of the immutables.
    function setImmutables(address _dest, ImmutableData[] calldata _immutables) external override {
        require(msg.sender == address(DEPLOYER_SYSTEM_CONTRACT), "Callable only by the deployer system contract");
        unchecked {
            uint256 immutablesLength = _immutables.length;
            for (uint256 i = 0; i < immutablesLength; ++i) {
                uint256 index = _immutables[i].index;
                bytes32 value = _immutables[i].value;
                immutableDataStorage[uint256(uint160(_dest))][index] = value;
            }
        }
    }

As you can see code doesn't keep record of immutables index and only set the values in the immutableDataStorage[][] mapping and in function getImmutable() code return the value from immutableDataStorage[][] which would return 0 value for non-existing immutable indexes. if any logic(inside a contract itself or in other contracts) access a contract non-existing(non-set) immutables it would get 0 value and it may run some logics based on those values. for example a contract is going to save variable isPrivate in and immutable index 5 (it is the 5th immutable), but because of some bug code doesn't set the value of the immutable but it access it, in the current implementation of the ImmutableSimulator when code access the immutable index 5(which is not set before) it would get 0 value and code would assume that contract is not private and allow some logics to be get called based on the immutable value. in the correct implementation the logic should have reverted because of accessing to non-existing immutable.

Tools Used

VIM

Recommended Mitigation Steps

code should keep track of max indexes for each address immutables (a mapping of address->uint256) and if the accessed immutable index in the getImmutable() was higher than that address index then it should revert.

[GalloDaSballo]

Am thinking this depends on the Sponsor goal (will need to look deeper)

If we strictly look at the Storage Implementation, then 0 is the appropriate value as that's what empty data is

If we look at it as a getter for non-existing function, then reverting would be more appropriate

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

vladbochok marked the issue as disagree with severity

[c4-sponsor]

vladbochok marked the issue as sponsor disputed

[vladbochok]

if any logic(inside a contract itself or in other contracts) access a contract non-existing(non-set) immutables it would get 0 value and it may run some logics based on those values.

In other words, if there is a bug in the contract, then in theory, when receiving an immutable non-existent variable, the user will receive a null value.

However, if there is a bug in a contract it already works wrong. It is also hard to say what will be better to return the default value or revert since it should never happen.

Based on that, I don't think it is a valid issue. The mitigation doesn't really help to resolve the described issue, and the issue itself refers to a potential bug in the compiler that is much more problematic.

[c4-sponsor]

vladbochok requested judge review

[GalloDaSballo]

I have considered Medium Severity as the behaviour could have been interpreted as "against the EVM spec"

However, upon further reflection, I believe that the behavior is consistent with how Solidity and the EVM work in general, the scenario shown by the Warden is the case of an immutable variable that is not initialized (else how would the contract access it?), and the "spec" behaviour for such a case is for the value of the variable to be interpreted as the 0 value, casted to whichever type Solidity is using.

For this reason, I think the finding is worth flagging, as a gotcha, but I believe no vulnerability was shown

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)