First Flight #19: Mondrian Wallet v2
First Flight #19
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Missing Access Control on `MondrianWallet2::executeTransaction` allows for breaking of a fundamental invariant of ZKSync: the uniqueness of (sender, nonce) pairs in transactions.

7Cedars

Missing Access Control on MondrianWallet2::executeTransaction allows for breaking of a fundamental invariant of ZKSync: the uniqueness of (sender, nonce) pairs in transactions.

Description: As the ZKSync documentation states:

One of the important invariants of every blockchain is that each transaction has a unique hash. [...]
Even though these transactions would be technically valid by the rules of the blockchain, violating hash uniqueness would be very hard for indexers and other tools to process. [...]
One of the easiest ways to ensure that transaction hashes do not repeat is to have a pair (sender, nonce) always unique.
The following protocol [on ZKSync] is used:

  • Before each transaction starts, the system queries the NonceHolder to check whether the provided nonce has already been used or not.

  • If the nonce has not been used yet, the transaction validation is run. The provided nonce is expected to be marked as "used" during this time.

  • After the validation, the system checks whether this nonce is now marked as used.

In short, for ZKSync to work properly, each transaction that is executed needs to have a unique (sender, nonce) pair. The MondrianWallet::validateTransaction function ensures this invariance holds, by increasing the nonce with each validated transaction.

Usually, ZKSync's bootloader of calls validate before executing a transaction and checks if a transaction has already been executed. However, because in MondrianWallet2 the owner of the contract can also execute a transaction, they can choose to execute a transaction multiple times - irrespective if this transaction has already been executed.

function executeTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction)

Impact: As the owner of MondrianWallet2 can execute a transaction multiple times, it breaks a fundamental invariant of ZKSync: the uniqueness of (sender, nonce) pairs. It can potentially have serious consequences for the functioning of the contract.

Proof of Concept:

  1. A user creates a transaction to mint usdc coins.

  2. The user executes the transaction, with nonce 0.

  3. The user executes the same transaction - again with nonce 0.

  4. And again, and again.

Proof of Concept

Place the following in ModrianWallet2Test.t.sol.

function testExecuteTransactionBreaksUniquenessNonce() public onlyZkSync {
vm.deal(address(mondrianWallet), AMOUNT);
uint256 amoundUsdc = 1e10;
uint256 numberOfRuns = 3; // the number of times to execute a transaction.
​
address dest = address(usdc);
uint256 value = 0;
bytes memory functionData = abi.encodeWithSelector(ERC20Mock.mint.selector, address(mondrianWallet), amoundUsdc);
Transaction memory transaction = _createUnsignedTransaction(mondrianWallet.owner(), 113, dest, value, functionData);
transaction = _signTransaction(transaction);
​
vm.startPrank(mondrianWallet.owner());
for (uint256 i; i < numberOfRuns; i++) {
// the nonce stays at 0.
vm.assertEq(transaction.nonce, 0);
// each time the execution passes without problem.
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
}
vm.stopPrank();
​
// this leaves the owner with 3 times the amount of usdc coins - because the contracts has been called three times. With the exact same sender-nonce pair.
assertEq(usdc.balanceOf(address(mondrianWallet)), numberOfRuns * amoundUsdc);
}

Recommended Mitigation: The simplest mitigation is to only allow the bootloader to call executeTransaction. This can be done by replacing the requireFromBootLoaderOrOwner modifier with the requireFromBootLoader modifier.

function executeTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction)
external
payable
+ requireFromBootLoader
​
function executeTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction)
external
payable
- requireFromBootLoaderOrOwner

This also allows for the deletion of the requireFromBootLoaderOrOwner modifier in its entirety:

- modifier requireFromBootLoaderOrOwner() {
- if (msg.sender != BOOTLOADER_FORMAL_ADDRESS && msg.sender != owner()) {
- revert MondrianWallet2__NotFromBootLoaderOrOwner();
- }
- _;
- }
Updates

Lead Judging Commences

bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

7Cedars Submitter
about 1 year ago
bube Lead Judge
12 months ago
bube Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.