Wrong Result When EcAdd Precompile Contract is Called in Delegatecall

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The observed inconsistency in the behavior of the EcAdd precompile contract when accessed via delegatecall introduces unpredictability within the zkSync Era environment. This inconsistency may affect the reliability and expected functionality of contracts using this precompile.

Proof of Concept

Within the zkSync Era environment, an inconsistency emerges regarding the behavior of the EcAdd precompile contract, found at address 0x06, when accessed through a delegatecall. This behavior diverges from the standard Ethereum Virtual Machine (EVM) operation, where the outcomes are consistent across call, staticcall, and delegatecall methods.
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/precompiles/EcAdd.yul#L5

In the zkSync Era, when the EcAdd precompile contract is invoked using delegatecall, it deviates from the typical behavior. Instead of performing as a precompile contract, it delegates the call to the body of the contract itself and executes its code within the caller's context. Consequently, the returned value does not align with the expected outcome of a precompileCall.

To illustrate this discrepancy, consider the following example. In the Ethereum Virtual Machine, when executing the provided code, the returned struct G1Point value consistently appears as follows:

{X: 1368015179489954701390400359078579693043519447331113978918064868415326638035, Y: 9918110051302171585080402603319702774565515993150576347155970296011118125764}

for all three scenarios: ecAddStaticcall, ecAddCall, and ecAddDelegatecall. However, in the zkSync Era, while ecAddStaticcall and ecAddCall produce the same results as in the EVM, ecAddDelegatecall yields an incorrect outcome.

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

struct G1Point {
    uint256 X;
    uint256 Y;
}

contract EcAddPoC {
    uint256[4] input = [1, 2, 1, 2];

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

        assembly {
            pop(staticcall(gas(), 0x06, _input, 0xc0, r, 0x60))
        }
    }

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

        assembly {
            pop(call(gas(), 0x06, 0, _input, 0xc0, r, 0x60))
        }
    }

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

        assembly {
            pop(delegatecall(gas(), 0x06, _input, 0xc0, r, 0x60))
        }
    }
}

This discrepancy is of particular significance because it deviates from the expected EVM response. While the likelihood of encountering this issue is not high, as precompile contracts are typically invoked through staticcall rather than delegatecall, it remains a point of concern 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(0x06){
            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]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

Similarly with other repots, the Warden has shown an inconsistent behaviour of a precompile, when using delegatecall
Because the behaviour differs from the EVM, I believe that Medium Severity is appropriate

[GalloDaSballo]

@miladpiri just pinging in case this was missed

[c4-sponsor]

miladpiri (sponsor) confirmed

[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