The value mapped under a nonce can be overwritten

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/NonceHolder.sol#L77-L96 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/NonceHolder.sol#L101-L104 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/NonceHolder.sol#L151-L166 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/bootloader/bootloader.yul#L2498-L2523

Vulnerability details

Impact

Specific inconsistent state caused by setters by sidestepping the checks in the codebase.

Once a nonce has been used, the value mapped under it shouldn't be changeable.

Proof of Concept

1) Some input validations exist in NonceHolder.setValueUnderNonce(). One prevents the nonce's value from being 0, the other hints at an attempt to have a sequentially used nonce.

2) However, it can be seen that there are no checks preventing a previously filled key to be used again and overwrite the _value that was used for it. Therefore, calling setValueUnderNonce() twice in a row, with the same valid _key but two different _value, is valid, effectively overwriting the value under nonce.

3) Given the fact that the method getValueUnderNonce() exists, it should then be expected that it returns the right value under the nonce, and not one that has been mistakenly overwritten at some point.

4) The impact here would be limited to a dirtied state as there's a validateNonceUsage() function called in the bootloader.

Coded POC

1) Use the following to setup a Foundry POC:

Setting up a hybrid project

yarn add --dev hardhat @nomicfoundation/hardhat-foundry

File: hardhat.config.ts
5: import "@nomicfoundation/hardhat-foundry";

npx hardhat init-foundry
forge remappings &> remappings.txt 

Set the following:

File: foundry.toml
+ via_ir = true

And, unfortunately, for the project / current scope to compile with Foundry so that we can test what we want: we need to delete the .yul files under contracts (precompiles/ and EventWriter.yul), otherwise we'll get an error telling us that the compiler only supports 1 yul file in the project.

2) Replace the following so that the code can compile with Foundry. We'll be forcing a Sequential nonce ordering here but it doesn't really matter:

diff --git a/contracts/NonceHolder.sol b/contracts/NonceHolder.sol
index 74e1dd6..47ddffc 100644
--- a/contracts/NonceHolder.sol
+++ b/contracts/NonceHolder.sol
@@ -79,12 +79,12 @@ contract NonceHolder is INonceHolder, ISystemContract {
     /// @param _value The value to store under the _key.
     /// @dev The value must be non-zero.
     function setValueUnderNonce(uint256 _key, uint256 _value) public onlySystemCall {
-        IContractDeployer.AccountInfo memory accountInfo = DEPLOYER_SYSTEM_CONTRACT.getAccountInfo(msg.sender);
+        // IContractDeployer.AccountInfo memory accountInfo = DEPLOYER_SYSTEM_CONTRACT.getAccountInfo(msg.sender);

         require(_value != 0, "Nonce value can not be set to 0");
         // If an account has sequential nonce ordering, we enforce that the previous
         // nonce has already been used.
-        if (accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential && _key != 0) {
+        if (true /* accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential */ && _key != 0) {
             require(isNonceUsed(msg.sender, _key - 1), "Previous nonce has not been used");
         }

3) Inject the following coded POC under 2023-03-zksync/test/nonceHolder.t.sol:

File: nonceHolder.t.sol
01: // SPDX-License-Identifier: UNLICENSED
02: pragma solidity ^0.8.13;
03: 
04: import "forge-std/Test.sol";
05: import {NonceHolder} from "contracts/NonceHolder.sol";
06: import {MAX_SYSTEM_CONTRACT_ADDRESS} from "contracts/Constants.sol";
07: 
08: contract NonceHolderTest is Test {
09:     NonceHolder internal nonceHolder;
10:     
11:     function setUp() public {
12:         nonceHolder = new NonceHolder();
13:     }
14: 
15:     function test_setValueUnderNonce() public {
16:         // The setter function have the `onlySystemCall` modifier
17:         vm.startPrank(address(MAX_SYSTEM_CONTRACT_ADDRESS));
18:         nonceHolder.setValueUnderNonce(0, 1337);
19:         console.log("nonceHolder.getValueUnderNonce(0): ", nonceHolder.getValueUnderNonce(0));
20:         nonceHolder.setValueUnderNonce(1, 1337);
21:         console.log("nonceHolder.getValueUnderNonce(1): ", nonceHolder.getValueUnderNonce(1));
22:         nonceHolder.setValueUnderNonce(2, 1337); 
23:         console.log("nonceHolder.getValueUnderNonce(2): ", nonceHolder.getValueUnderNonce(2));
24:         nonceHolder.setValueUnderNonce(1, 42);
25:         console.log("nonceHolder.getValueUnderNonce(1): ", nonceHolder.getValueUnderNonce(1));
26:         nonceHolder.setValueUnderNonce(0, 42);
27:         console.log("nonceHolder.getValueUnderNonce(0): ", nonceHolder.getValueUnderNonce(0));
28:     }
29: }
30: 

4) Run the test with forge test -m test_setValueUnderNonce -vv and see the following output:

Running 1 test for test/nonceHolder.t.sol:NonceHolderTest
[PASS] test_setValueUnderNonce() (gas: 177853)
Logs:
  nonceHolder.getValueUnderNonce(0):  1337
  nonceHolder.getValueUnderNonce(1):  1337
  nonceHolder.getValueUnderNonce(2):  1337
  nonceHolder.getValueUnderNonce(1):  42
  nonceHolder.getValueUnderNonce(0):  42

Test result: ok. 1 passed; 0 failed; finished in 542.33µs

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Given that isNonceUsed() will return true if there's a non-zero value under the nonce and there's a Reusing the same nonce twice error in validateNonceUsage, we can see here that an existing path to taint values under a nonce shouldn't exist.

In setValueUnderNonce(), consider enforcing the unique operation of "writing a value under nonce" by adding a check similar to the following:

    require(!isNonceUsed(msg.sender, _key), "Current nonce has already been used");

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Primary because of coded POC

[miladpiri]

Intended by design. The value can be used for any purposes by the owner of the account. The other users who interacts with this value should know how the system works.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

I have considered Med and QA for the report, but I must close as OOS because the sponsor has documented these gotchas in the readme, please see the following

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/README.md#L633-L639

It provides a function `validateNonceUsage` which the bootloader uses to check whether the nonce has been used for a certain account or not. Bootloader enforces that the nonce is marked as non-used before the validation step of the transaction and marked as used afterward. The contract ensures that once marked as used, the nonce can not be set back to the "unused" state. 

Note that nonces do not necessarily have to be monotonic (this is needed to support more interesting applications of account abstractions, e.g. protocols that can start transactions on their own, tornado-cash like protocols, etc). That's why there are two ways to set a certain nonce as "used":

- By incrementing the `minNonce` for the account (thus making all nonces that are lower than `minNonce` as used).
- By setting some non-zero value under the nonce via `setValueUnderNonce`. This way, this key will be marked as used and will no longer be allowed to be used as the nonce for accounts. This way it is also rather efficient since these 32 bytes could be used to store some valuable information.

As the sponsor said:

• Nonce may not be monotonic, and may even be re-used
• Bootloader verifies the correctness of usage of the nonce

None of the statement was invalidated by the report and for this reason, in-spite of the quality of the submission I must close as OOS as already disclosed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope