taikoxyz / taiko-mono

A based rollup. 🥁
https://taiko.xyz
MIT License
4.16k stars 1.99k forks source link

chore(protocol): delete buggy Lib4844.sol #16969

Closed dantaik closed 2 weeks ago

dantaik commented 2 weeks ago

The implementation is buggy and we'll not use it in zkVM proofs.

from OZ:

The evaluatePoint function is used to validate that the opening of a KZG Commitment (corresponding to a blob in practice) at a point x is equal to the purported y by calling Ethereum's point evaluation precompile. This precompile expects its commitment and proof arguments to be 48 bytes each.

However, the _commitment and _pointProof arguments are of type bytes1[48], and passed by calling abi.encodePacked. The packed encoding of a bytes1[48] is done in-place without the length but with each element padded to 32 bytes. This means that the commitment and proof arguments sent to the precompile are 32 * 48 = 1536 instead of 48 bytes long, and thus the first padded 3 bytes of the commitment are erroneously interpreted as representing both the commitment and the proof. Because these are padded and mostly 0s, they are unlikely to correspond to a valid commitment/proof pair.

In practice, this means that any normal call to the point evaluation precompile would revert. Additionally, gas could be saved by changing the type of these arguments to bytes and validating their length separately. We note that while the Lib4844 library is in the scope of this audit, it is not currently used by the contracts in scope.

Consider changing the _commitment and _pointProof arguments to be of type bytes. Additionally, if the Lib4844 is intended to be used in practice, consider adding tests targeting this library.

dantaik commented 2 weeks ago

@Brechtpd I don't think this lib will every be used in zkVM proofs, but let me know otherwise.

openzeppelin-code[bot] commented 2 weeks ago

Delete Lib4844.sol

Generated at commit: 95b3aa8c46c57df74d0c7a17013d2df43e83d524

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
5
40
49
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

dantaik commented 2 weeks ago

There is a small chance we may still use the precompile as an optimization at some point, but that's getting unlikelier by the day. So I also don't see a reason to keep it in the main repo.

OK, then lets delete it for know. We can revert this commit then fix the bug if we change our mind.