c4-bot-5 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/precompiles/P256Verify.yul#L76

Vulnerability details

Impact

Incorrect unsafePackPrecompileParams in P256Verify.yul might result in invalid return value.

Proof of Concept

In P256Verify.yul, the secp256r1 circuit is expected to return a two word return value - 1st word: internalSuccess(bool); 2nd word: actual writtenValue. And a call to this precompile is expected to return the 2nd word writtenValue.

This can be seen from the final return(32,32) statement.

However, unsafePackPrecompileParams() is currently incorrectly coded. Notably, the output length in words in hardcoded as 1, which will return only 1 word at the memory 0 location. Based on this, nothing is instructed to be returned at 32.

            let precompileParams := unsafePackPrecompileParams(
                0, // input offset in words
                5, // input length in words (the signed digest, r, s, x, y)
                0, // output offset in words
|>              1, // output length in words (success) //@audit This should be 2, indicating two word length
                0  // No special meaning, secp256r1 circuit doesn't check this value
            )

...
            default {
                // The circuits might write `0` to the memory, while providing `internalSuccess` as `1`, so
                // we double check here.
                let writtenValue := mload(32)
                if eq(writtenValue, 0) {
                    return(0, 0)
                }
                 //@audit actual writtenValue is returned at 32, the second word. 
|>               return(32, 32)
            }

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/precompiles/P256Verify.yul#L76)

Incorrect hardcoded precompileParams might cause future incompatibilities with secp256r1 circuit, and return invalid value.

For comparison, current Ecrecover.yul hardcoded the correct parameter for second-word return.

Tools Used

Manual

Recommended Mitigation Steps

Change output length to 2.

Assessed type

Error

c4-sponsor commented 5 months ago

saxenism marked the issue as disagree with severity

saxenism commented 5 months ago

We consider this issue QA at best because the value is currently ignored.

c4-sponsor commented 4 months ago

razzorsec (sponsor) acknowledged

alex-ppg commented 4 months ago

The Warden specifies that the P256Verify precompile will misbehave as it will never yield the writtenValue to its caller. I would normally classify this as a medium-risk vulnerability despite the Sponsor's statement, however, I investigated the RIP-7212 proposal that the precompile is meant to implement and it denotes that the precompile should yield the success flag rather than the written value.

While the present implementation is "incorrect", it complies with the RIP-7212 (having the same bearing as EIPs) it is meant to implement as the following code:

let writtenValue := mload(32)
if eq(writtenValue, 0) {
    return(0, 0)
}

Effectively yields the success bool at all times that is returned from the precompile. As such, I consider this submission to unfortunately be of QA level despite highlighting an actual issue with the code.

c4-judge commented 4 months ago

alex-ppg changed the severity to QA (Quality Assurance)