code-423n4 / 2023-10-zksync-findings

4 stars 0 forks source link

EIP-1559 transactions can be invoked from kernel space accounts due to missing assertion in bootloader #224

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2903-L2905

Vulnerability details

Impact

The validateTypedTxStructure(...) function in the bootloader checks invariants for multiple transaction types. When the bootloader is in live / production mode, i.e. BOOTLOADER_TYPE=='proved_batch', those checks are even stricter and therefore enforce that a transaction's from address is not part of the kernel space.

Links to the aforementioned kernel space checks:

Due to proved_block instead of proved_batch, the critical assertion is not included in the bootloader production build in case of EIP-1559 transactions and therefore allows L2 transactions to be invoked from kernel space addresses.

Proof of Concept

Part 1 Run bash quick-setup.sh or specifically run yarn preprocess && yarn compile-yul from ./code/system-contracts/scripts to review that the resulting production bootloader ./code/system-contracts/bootloader/build/proved_batch.yul in fact does not contain the critical kernel space assertion for EIP-1559 transactions (type = 2).

Part 2 The following PoC demonstrates that transactions from kernel space are possible due to the missing assertion in case of EIP-1559 transactions (type = 2).

Apply the diff to the existing DefaultAccount test suite:

diff --git a/code/system-contracts/test/DefaultAccount.spec.ts b/code/system-contracts/test/DefaultAccount.spec.ts
index 6231c34..70d3087 100644
--- a/code/system-contracts/test/DefaultAccount.spec.ts
+++ b/code/system-contracts/test/DefaultAccount.spec.ts
@@ -152,6 +152,26 @@ describe('DefaultAccount tests', function () {
     });

     describe('executeTransaction', function () {
+        it.only('from kernel space', async () => {
+            const kernelSpaceAddress = "0x000000000000000000000000000000000000dEaD"; // must be < 0xFFFF
+
+            // get impersonated signer for address
+            await network.provider.request({
+                method: 'hardhat_impersonateAccount',
+                params: [kernelSpaceAddress]
+            });
+            const kernelSpaceSigner = ethers.provider.getSigner(kernelSpaceAddress);
+
+            // do transaction from kernel space
+            await callable.connect(kernelSpaceSigner).fallback({type: 2});
+
+            // stop address impersonation
+            await network.provider.request({
+                method: 'hardhat_stopImpersonatingAccount',
+                params: [kernelSpaceAddress]
+            })
+        });
+
         it('non-deployer ignored', async () => {
             let nonce = await nonceHolder.getMinNonce(account.address);
             const legacyTx = await account.populateTransaction({

Run bash quick-setup.sh from ./code/system-contracts/scripts to execute the PoC test case:

 1) DefaultAccount tests
       executeTransaction
         from kernel space:
     Execution error: Transaction HALT: Account validation error: Invalid v value

Although the account validation fails due to the revert in BootloaderUtilities.sol:L286, since the signature of Hardhat impersonated accounts is - of course - invalid, one can see that the kernel space assertion was successfully "bypassed" (it is missing), because the reverted call to BootloaderUtilities from the bootloader's saveTxHashes(...) function happens during the L2 transaction validation step, see bootloader.yul:L1190, which is way after the incoming transaction structure check where the the aforementioned assertions happen.

Tools Used

Manual review

Recommended Mitigation Steps

Replace proved_block with proved_batch in L2903 of the bootloader.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 1 year ago

bytes032 marked the issue as duplicate of #898

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)