Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c4-submissions opened this issue Oct 17, 2023 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-898 edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Oct 17, 2023

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-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 17, 2023
c4-submissions added a commit that referenced this issue Oct 17, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Oct 31, 2023
@c4-pre-sort c4-pre-sort added duplicate-898 and removed primary issue Highest quality submission among a set of duplicates labels Oct 31, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #898

@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Oct 31, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 25, 2023
@C4-Staff C4-Staff reopened this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-898 edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants