code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/precompiles/Ecrecover.yul#L62

Vulnerability details

Impact

The Ecrecover.yul file meant to simulate the ecrecover mechanism as executed by traditional ETH 1.0 consensus mechanisms is incorrect. In detail, it does not conform to the "Homestead" update which introduced an upper-bound check for s values of an (r, s, v) ECDSA payload to prohibit malleable signatures.

All major ETH 1.0 clients (i.e. go-ethereum, nethermind) prevent s values as input in ecrecover instructions if the value lies in the upper half of the secp256k1 curve order. To illustrate:

The consensus discrepancy at hand illustrates a security feature present in ETH 1.0 clients and absent in the zkEVM. As such, a contract that would be secure when deployed in ETH 1.0 will not be so when deployed in the zkEVM. This vulnerability allows malleable signatures to be consumed thus opening up potential replay-attack vectors that would otherwise be impossible in ETH 1.0.

The ecrecover function lies at the heart of multiple authorization systems in use by smart contracts, meaning that a significant amount of funds would be affected by this discrepancy.

Proof of Concept

An ecrecover call with an s value that is greater than or equal to 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1 should fail execution, however, the operation properly succeeds in the zkEVM. A minimalistic contract that illustrates the issue:

contract ECRecoverVulnerability {
    function test(uint8 v, uint256 r, uint256 s) external view {
        require(s >= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1, "S value be in upper-half of secp256k1 curve order");
        ecrecover(r, s, v);
        revert("ISSUE: Should not have reached here");
    }
}

Tools Used

Manual review, ETH 1.0 clients, Ethereum Foundation release guidelines.

Recommended Mitigation Steps

We advise the upper bound evaluation to be performed similarly to the SECP256K1_GROUP_SIZE bound check already applied to the codebase, utilizing the nethermind coding style which divides the group size by 2.

miladpiri commented 1 year ago

The malleability checks are on the client side on Ethereum (and are done by the DefaultAсcount.sol). https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L187

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

@miladpiri to confirm: Are you saying that MM will reject the signature or is the check going to be performed by the zkSync node (I believe it should be Core)?

vladbochok commented 1 year ago

@GalloDaSballo I believe that Milad meant the ecrecover precompile on EVM accepts the malleable signature, but specifically, the transaction signature is accepted as not malleable.

On zkSync Era, we also accept the malleable signature on ecrecover. But the default account (analog to the Ethereum EOA) will reject signature malleavility. So all good

GalloDaSballo commented 1 year ago

I must agree with the sponsor:

ecRecover on the Node will revert

The Precompile ecrecover will not revert if the value is above half the curve, see below fuzzed test:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.15;

import {Test} from "forge-std/Test.sol";

contract TestDirty is Test {

    function setUp() public {
    }

    function testEC(uint8 v, uint256 r, uint256 s) external view {
          s = s % 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1;
          s += 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1;
          ecrecover(bytes32(0), v, bytes32(r), bytes32(s));
    }

}

This test passes on mainnet meaning that the behaviour is consistent between the two implementations

The AA is safe from malleability per the check shown by the Sponsor

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

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