EcMul Precompile Contract Behaves Differently when Using Delegatecall

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/precompiles/EcMul.yul#L5

Vulnerability details

Impact

The behavior of the EcMul precompile contract in zkSync Era, when accessed through delegatecall, deviates from the expected Ethereum Virtual Machine (EVM) behavior.

Proof of Concept

In the context of zkSync Era, an inconsistency has surfaced in the behavior of the EcMul precompile contract, which is located at address 0x07, when it is accessed through a delegatecall. This behavior differs from the standard behavior observed within the Ethereum Virtual Machine (EVM), where results remain consistent regardless of whether you use call, staticcall, or delegatecall methods.
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/precompiles/EcMul.yul#L5

In zkSync Era, when the EcMul precompile contract is invoked using a delegatecall, it behaves like a normal delegate call in which the code is executed within the context of the caller. As a consequence, the value returned from this operation does not align with the expected result of a precompileCall.

To illustrate this inconsistency, consider the following example: In the Ethereum Virtual Machine, running the provided code consistently produces a struct G1Point with the following values:

{X: 3353031288059533942658390886683067124040920775575537747144343083137631628272, Y: 19321533766552368860946552437480515441416830039777911637913418824951667761761}

This consistent behavior is observed for all three scenarios: ecMulStaticcall, ecMulCall, and ecMulDelegatecall. However, in the zkSync Era, while ecMulStaticcall and ecMulCall exhibit the same results as in the EVM, ecMulDelegatecall produces an incorrect outcome, which is zero.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

struct G1Point {
    uint256 X;
    uint256 Y;
}

contract EcMulPoC {
    uint256[3] input = [1, 2, 3];

    function ecMullStaticcall() public returns (G1Point memory r) {
        uint256[3] memory _input = input;

        assembly {
            pop(staticcall(gas(), 0x07, _input, 0x80, r, 0x60))
        }
    }

    function ecMullDelegatecall() public returns (G1Point memory r) {
        uint256[3] memory _input = input;

        assembly {
            pop(delegatecall(gas(), 0x07, _input, 0x80, r, 0x60))
        }
    }

    function ecMullCall() public returns (G1Point memory r) {
        uint256[3] memory _input = input;

        assembly {
            pop(call(gas(), 0x07, 0, _input, 0x80, r, 0x60))
        }
    }
}

This discrepancy carries significance because it deviates from the expected behavior within the Ethereum Virtual Machine. It is worth noting that the likelihood of encountering this issue is not high in practice, as precompile contracts are typically invoked using staticcall rather than delegatecall. Nevertheless, this finding is noteworthy as it highlights an inconsistency that should be addressed and understood within the zkSync Era environment.

Tools Used

Recommended Mitigation Steps

The following revised code is recommended:

function delegateCall(
        uint256 _gas,
        address _address,
        bytes calldata _data
    ) internal returns (bytes memory returnData) {
        bool success;
        if(_address == address(0x07){
            success = rawStaticCall(_gas, _address, _data);
        } else {
            success = rawDelegateCall(_gas, _address, _data);
        }
        returnData = _verifyCallResult(success);
    }

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/libraries/EfficientCall.sol#L88

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[miladpiri]

High Impact (as result is false), low probablity (usually static call is used). There is no proof that anyone need this behaviour. Medium is fair.

[c4-sponsor]

miladpiri (sponsor) confirmed

[GalloDaSballo]

The Warden has shown an inconsistency in the behaviour of EcMul when using delegatecall

Because the goal of the zkSync EVM is to be the EVM compatible, Medium Severity seems appropriate

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

I have spent quite some time to verify the statement made and believe this finding to be invalid

The ecMul Precompile is stateless, it will not look at the caller

The call will behave as intended in either way

I have verified my statement through testing via the zksync-foundry code repo as well as reviewing both the Precompile as well as the Rust code

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Invalid