[Medium - 1] Ecrecover precompile doesn't behave the same as the one from Ethereum

[code423n4]

Lines of code

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

Vulnerability details

Impact

According to the Ethereum yellow paper and in the specifications of the ecrecover precompile, it is stated that if the ecrecover doesn't return anything (denoted by ∅), then the return should be 0 as well.

If we take a look at the current ecrecover implementation of the precompile, it does a precompileCall with the signature whose parameters (r, s, v) were checked before. Then loads the memory at slot 0 and we can see just after that in order to return some data, those two must be true.

The issue is that compared to the yellow paper, there is no handling of the case when the precompile doesn't return anything, but yields a success boolean.

As the client is not in scope, it is not clear how it would result in if there was a bug there as well.

Then, it may return the data that is at the location 32 or 0x20 in hex for a length of 32, most likely the v parameter which was substracted by 1. This is not what we would expect from the result of a ecrecover call, that should normally return the address of the signer of the message.

This is where it is handled in go-ethereum, the Ethereum client: https://github.com/ethereum/go-ethereum/blob/master/crypto/secp256k1/secp256.go#L118

// RecoverPubkey returns the public key of the signer.
// msg must be the 32-byte hash of the message to be signed.
// sig must be a 65-byte compact ECDSA signature containing the
// recovery id as the last element.
func RecoverPubkey(msg []byte, sig []byte) ([]byte, error) {
    if len(msg) != 32 {
        return nil, ErrInvalidMsgLen
    }
    if err := checkSignature(sig); err != nil {
        return nil, err
    }

    var (
        pubkey  = make([]byte, 65)
        sigdata = (*C.uchar)(unsafe.Pointer(&sig[0]))
        msgdata = (*C.uchar)(unsafe.Pointer(&msg[0]))
    )
  // ZkSync-era audit: if the recover return is nothing, return an error
    if C.secp256k1_ext_ecdsa_recover(context, (*C.uchar)(unsafe.Pointer(&pubkey[0])), sigdata, msgdata) == 0 {
        return nil, ErrRecoverFailed
    }
    return pubkey, nil
}

Tools Used

Manual inspection

Recommended Mitigation Steps

Check that the returndatasize isn't zero as another parameter to return some data.

// Check whether the call is successfully handled by the ecrecover circuit
let success := precompileCall(precompileParams, gasToPay)
let internalSuccess := mload(0)
let size := returndatasize()

switch and(and(success, internalSuccess), size)
case 0 {
    return(0, 0)
}
default {
    return(32, 32)
}

[miladpiri]

AFAIU, warden says that the innerSuccess may be true, but the returned size is zero. It looks off.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[c4-sponsor]

miladpiri requested judge review

[GalloDaSballo]

Logically speaking

ecrecover returns 0, and returndatasize is non-zero (current code):

• We return 0 (consistent with spec even though we returned a non-empty set, as the empty set is casted / represented by empty bytes)

ecrecovers returns 0, and returndatasize is zero:

• We still return 0 as the value in the 32 bytes in memory is still zero, even though it's empty as it wasn't assigned any value due to ecrecover returning a 0 value

For the logic above, I believe that the implementations are identical (semantically different but behaviors are the same), meaning that the finding is invalid

Please @iFrostizz feel free to send a coded POC of a different behavior if you can reproduce during Post Judging QA

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[iFrostizz]

Hey @GalloDaSballo !

Thanks for your review, The issue with the precompile return empty bytes with zero length but still yields a success is that the ecrecover zk precompile will the load the bytes from the memory (those which should have been written by the precompile) at the key 32. Meaning that it will return sub(v, 27) defined above, thus why the ecrecover is inconsistent and might lead to some funky behaviour.

[GalloDaSballo]

Hey @GalloDaSballo !

Thanks for your review, The issue with the precompile return empty bytes with zero length but still yields a success is that the ecrecover zk precompile will the load the bytes from the memory (those which should have been written by the precompile) at the key 32. Meaning that it will return sub(v, 27) defined above, thus why the ecrecover is inconsistent and might lead to some funky behaviour.

Thank you for the extra info @iFrostizz can you please provide me with a coded POC of at least one instance that would cause this to happen?

[iFrostizz]

Hey @GalloDaSballo ! Thanks for your review, The issue with the precompile return empty bytes with zero length but still yields a success is that the ecrecover zk precompile will the load the bytes from the memory (those which should have been written by the precompile) at the key 32. Meaning that it will return sub(v, 27) defined above, thus why the ecrecover is inconsistent and might lead to some funky behaviour.

Thank you for the extra info @iFrostizz can you please provide me with a coded POC of at least one instance that would cause this to happen?

Right so I've done a small repro. Calling a contract that returns nothing but with a successful return:

assembly {
    return(0, 0)
}

Will still allocate empty memory, so will overwrite the v in the memory as the retLen of the call was set to 0x20. Though, according to the yellow paper and shown in the report, the length of the return data of the Ecrecover may be 0 (denoted by ||o||) if the return data of the ecrecover is none even if the inner call is true.

This is a POC: https://github.com/iFrostizz/ecrecover-mock

It tests for the unhappy path of an empty return data with a successful inner call, while the call shouldn't be yielding true.

[GalloDaSballo]

@iFrostizz If we are returning a size of 0, then by definition aren't we returning a empty value which will be casted to 0?

Meaning we are returning the intended value?

[GalloDaSballo]

I would maintain invalid unless we can have a case where we'd have ecrecover's EVM implementation returning 0 and the zkEVM implementation returning a non-zero value

@iFrostizz Please do provide with such an instance and we'll re-judge (or follow up with the Sponsor privately once you do)

[iFrostizz]

Yep @GalloDaSballo , agreeing. I'll dig a bit more into the ecrecover inners and let you know