zksync ecrecover circuit recover random EOA address if the r and s value is random or invalid or malformed instead of return address(0)

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/ecrecover/mod.rs#L910 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/ecrecover/mod.rs#L936

Vulnerability details

Impact

zksync ecrecover circuit recover random address if the r and s value is random and invalid

Proof of Concept

zksync ecrecover circuit does not handle the v value properly

the ecrecover should take 4 input, the signature composed of r, v and s and a digest

if the valid signature is provided, the returned result should a valid EOA adress

if a invalid signature is provided, the result should return address(0)

this is the expected behavior of the ecrecover

there are other resources but we can use openzeppelin implementation as a reference

if a random invalid address is provided, instead of return address(0), a random EOA address is returned

the v value when testing is set in this line of code

let rec_id = UInt8::allocate_checked(cs, 0);

the v value is set to 0

this rec_id is the v value and it is used in this line of code

let (no_error, digest) = ecrecover_precompile_inner_routine(
    cs,
    &rec_id,
    &r,
    &s,
    &digest,
    valid_x_in_external_field.clone(),
    valid_y_in_external_field.clone(),
    valid_t_in_external_field.clone(),
    &base_params,
    &scalar_params,
);

the four input, v, r, s and digest is constructed here:

let digest_u256 = repr_into_u256(digest.into_repr());
let r_u256 = repr_into_u256(r.into_repr());
let s_u256 = repr_into_u256(s.into_repr());

let rec_id = UInt8::allocate_checked(cs, 0);
let r = UInt256::allocate(cs, r_u256);
let s = UInt256::allocate(cs, s_u256);
let digest = UInt256::allocate(cs, digest_u256);

if we just change the r or s to a random invalid value

for example, we change the r value

let r_u256 = repr_into_u256(r.into_repr()) + 10000;

and we need to comment out this line of code

// assert!(no_error.witness_hook(&*cs)().unwrap() == true);

we run the test

cargo test test_signature_for_address_verification

note the output is

assertion `left == right` failed
  left: [191, 210, 1, 82, 116, 3, 247, 77, 226, 58, 210, 142, 89, 61, 170, 168, 143, 11, 96, 3]
 right: [18, 137, 13, 44, 206, 16, 34, 22, 100, 76, 89, 218, 229, 186, 237, 56, 13, 132, 131, 12]

the testing code is

// assert!(no_error.witness_hook(&*cs)().unwrap() == true);
let recovered_address = digest.to_be_bytes(cs);
let recovered_address = recovered_address.witness_hook(cs)().unwrap();
assert_eq!(&recovered_address[12..], &eth_address[..]);

note the expected eth address &eth_address[..] hex decimal is

12890d2cce102216644c59dae5baed380d84830c

and the address is hardcoded before the test

this address value map to the right value

 right: [18, 137, 13, 44, 206, 16, 34, 22, 100, 76, 89, 218, 229, 186, 237, 56, 13, 132, 131, 12]

the left value is

 left: [191, 210, 1, 82, 116, 3, 247, 77, 226, 58, 210, 142, 89, 61, 170, 168, 143, 11, 96, 3]

but we are providing a purely random r, the recovered address is expected to be address(0), which should be

 left: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

the impact is if the input provide random r, v and s or modified v and s, while the code expected to return address(0)

the ecrecover circuit return to a wrong EOA address

Tools Used

Manual Review

Recommended Mitigation Steps

return address(0) instead of a random EOA address if r or s is modified or malformed

Assessed type

Invalid Validation

[shamatar]

Linked code is tests. But construction of requests to ecrecover circuit by precompile code at the corresponding address in VM we know that v is 0/1

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Downgrading to QA as the check is not in the circuit but the system as a whole is safe

[JeffCX]

from judge

the check is not in the circuit but the system as a whole is safe

from sponsor

Linked code is tests. But construction of requests to ecrecover circuit by precompile code at the corresponding address in VM we know that v is 0/1

but in this line of code (this is not from test code and this LOC linked is in the original submission)

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/ecrecover/mod.rs#L910

the circuit code does not validate if the v value is 0 or 1, it could be set to any value

such check should be enforced in circuit level,

I thought other bug report that point out missing check / constraint is judged as H / M

https://github.com/code-423n4/2023-10-zksync-findings/issues/1133

the circuit needs to verify that the remainder is less than the divisor

so even when precompile code knows v value, the v value should be validated on circuit level and so please consider the report as a medium severity report along with issue #674

thanks

I do respect judge's final decision!

[miladpiri]

from judge

the check is not in the circuit but the system as a whole is safe

from sponsor

Linked code is tests. But construction of requests to ecrecover circuit by precompile code at the corresponding address in VM we know that v is 0/1

but in this line of code (this is not from test code and this LOC linked is in the original submission)

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/ecrecover/mod.rs#L910

the circuit code does not validate if the v value is 0 or 1, it could be set to any value

such check should be enforced in circuit level,

I thought other bug report that point out missing check / constraint is judged as H / M

1133

the circuit needs to verify that the remainder is less than the divisor

so even when precompile code knows v value, the v value should be validated on circuit level and so please consider the report as a medium severity report along with issue #674

thanks

I do respect judge's final decision!

The finding just says we return a random address when given an invalid signature. This is intended behavior of the ecrecover circuit, as described in our documentation:

A special note about this circuit is that there are hardcoded ‘valid’ field element values provided to the circuit. This is to prevent the circuit from not satisfying in case the user-provided inputs are incorrect and, when the circuit detects this, the bad values are swapped out for the hardcoded ones. In this event, exceptions are logged and pushed into a vector which are returned to the caller, informing them that the provided inputs were incorrect and the result should be discarded. https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/docs/Circuits%20Section/Circuits/Ecrecover.md#main-circuit-logic

So, this is an invalid finding.

[JeffCX]

I really appreciate sponsor review the report one more time

I left more comments on https://github.com/code-423n4/2023-10-zksync-findings/issues/674

should be related to this issue as well

The finding just says we return a random address when given an invalid signature. This is intended behavior of the ecrecover circuit, as described in our documentation:

but in standard ecrecover, random address should not be returned

if a signature is invalid, address(0) should be returned instead of a random address

again the POC is in the original report

in this line of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/ecrecover/mod.rs#L911

 let r = UInt256::allocate(cs, r_u256);

we set the r to a random value

let r_u256 = repr_into_u256(r.into_repr()) + 10000;

and comment out

// assert!(no_error.witness_hook(&*cs)().unwrap() == true);

the log output is

assertion `left == right` failed
  left: [191, 210, 1, 82, 116, 3, 247, 77, 226, 58, 210, 142, 89, 61, 170, 168, 143, 11, 96, 3]
 right: [18, 137, 13, 44, 206, 16, 34, 22, 100, 76, 89, 218, 229, 186, 237, 56, 13, 132, 131, 12]

sure the code make sure the witness_hook is not true

but address(0) should be return along with false state of hook instead of returning random address

[JeffCX]

I was wondering if the sponsor comments refers to this check

Linked code is tests. But construction of requests to ecrecover circuit by precompile code at the corresponding address in VM we know that v is 0/1

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

            // Validate the input by the yellow paper rules (Appendix E. Precompiled contracts)
            let vIsInvalid := iszero(or(eq(v, 27), eq(v, 28)))
            let sIsInvalid := or(eq(s, 0), gt(s, sub(SECP256K1_GROUP_SIZE(), 1)))
            let rIsInvalid := or(eq(r, 0), gt(r, sub(SECP256K1_GROUP_SIZE(), 1)))

            if or(vIsInvalid, or(sIsInvalid, rIsInvalid)) {
                return(0, 0)
            }

but this report is saying a random invalid r, s should return address(0) instead of random EOA address

[JeffCX]

Hello, I write further test to test the behavior of ecrecover

but this report is saying a random invalid r, s should return address(0) instead of random EOA address

my claim is wrong, I think if we modify r or s, it will yield random invalid result for ecrecover as well

so this report is not valid, I agree with sponsor's claim and judge's assessment,

sorry for the confusion