We audited pull request #41 of the matter-labs/era-system-contracts repository at commit 217e115.
In scope were the following contracts:
├── bootloader
│ └── bootloader.yul
└── contracts
├── ContractDeployer.sol
└── precompiles
└── Keccak256.yul
The system upgrade introduces a new, more efficient way for the keccak256 function to work. In the previous system version, the keccak256 precompile process involved copying data into memory and padding it for processing, leading to additional processing steps and user charges for memory growth.
In the revised version, the keccak256 precompile process remains similar, but with the key change of removing the data copying and padding. The padding will now be handled within the ZK circuits. The new algorithm involves receiving data to hash and packing the parameters into the correct precompile ABI format.
The update also includes the implementation of the keccak256 upgrade process. When transitioning to a VM version that supports the new keccak256 precompile, the old keccak256 precompile will cease to function. Therefore, the initial keccak256 upgrade cannot involve any keccak256 invocations.
keccak256 relies on the out-of-scope keccak256 ZK circuit. Thus, it is assumed that the circuit is implemented correctly and behaves according to its specification.Keccak256 Contract Is Missing Constructor CodeThe Keccak256 contract defined in the Keccak256.yul file is missing the constructor definition, whereas all other precompile contracts have it.
Consider adding constructor code to the Keccak256 contract to improve its readability and clarity.
Update: Resolved in pull request #57 at commit bd52d32.
The upgrade to the new version of keccak256 lacks several tests that could significantly enhance the quality of the codebase. Consider adding the following tests to ensure the correctness of the upgrade process:
keccak256. The current upgrade process does not test transitioning from one keccak256 version to another. Instead, it acquires the current keccak256 version, changes it to a reverting contract, and then changes it back to the current version.keccak256 unit test to verify the correctness of generated hashes. Consider implementing fuzz testing for this purpose.Update: Resolved in pull request #58 at commit a4dc0d9.
The comment on lines 233 and 234 suggests that the forceDeployKeccak256 function is temporarily needed to upgrade the Keccak256 precompiled contract, and is to be removed in the future.
In the future, if forceDeployKeccak256 is removed, its interdependent counterpart upgradeKeccakIfNeeded function should also be removed. However, the documentation for the upgradeKeccakIfNeeded function does not mention that this is temporarily needed and should be removed when the upgrade process is no longer needed.
Consider adding a comment to the upgradeKeccakIfNeeded function, pointing out that it should be removed once the upgrade process is complete. Furthermore, ensure that the documentation for both mentioned functions includes a note specifying that their interdependent counterpart function should also be removed.
Update: Resolved in pull request #60 at commit 40d31d5.
The Keccak256.yul contract defines two magic hardcoded numbers, but there is a lack of proper documentation regarding how they were derived.
The BLOCK_SIZE function returns a value of 136, which is expected to be the size of the processing keccak256 block in bytes. Keccak256 is defined with rate (equivalent to "block size") $r = 1088$ bits $= 136$ bytes and capacity $c = 512$ bits $= 64$ bytes. The sum of the two gives the full size of the state of the Keccak permutation $r + c = 1600$ bits $= 200$ bytes. Clearly, the hard-coded constant $136$ corresponds to the size of the rate in bytes, but a detailed explanation of how it was derived is missing.
The KECCAK_ROUND_GAS_COST function returns a value of 40, which is expected to be the gas cost of processing one keccak256 round. Yet there is no explanation of how this value is computed.
Consider including explanations in the comments as to how the values $136$ and $40$ were derived.
Update: Resolved in pull request #61 at commit a90e2c0.
The ordering of functions that return contract addresses in bootloader.yul is not currently done based on the address value. This lack of proper ordering may lead to errors and issues when new contract addresses need to be added.
Consider reordering these functions according to the address they return for better organization and reliability.
Update: Resolved in pull request #62 at commit d6c93ed.
The proposed implementation of the keccak256 precompile calculates the amount of gas to be charged for hashing. This calculation is based on the number of keccak256 rounds required to process the given data, multiplied by the hardcoded cost of gas for a single round. The number of rounds is determined by performing a ceiling division on the data length with BLOCK_SIZE as the denominator.
This approach results in an issue: if empty data is hashed, the calculation yields a numRounds value of 0. While this is technically accurate because no rounds of hashing are executed, there is still some processing involved in returning the keccak256 hash for empty input data.
Consider charging gas for cases where empty data is being hashed.
Update: Resolved in pull request #63 at commit 4f1b6c6.
The audit did not reveal any significant issues with the keccak256 upgrade. Various recommendations have been provided to enhance the quality and documentation of the codebase. The Matter Labs team was responsive and facilitated the setup of a functional test environment for running the test code.