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

Lacking control on return data at `MondrianWallet2::_executeTransaction` results in excessive gas usage, unexpected behaviour and unnecessary evm errors.

7Cedars

Lacking control on return data at MondrianWallet2::_executeTransaction results in excessive gas usage, unexpected behaviour and unnecessary evm errors.

Description: The _executeTransaction function uses a standard .call function to execute the transaction. This function returns a bool success and bytes memory data.

However, ZKsync handles the return of this the bytes data differently than on Ethereum mainnet. In their own words, from the ZKsync documentation:

unlike EVM where memory growth occurs before the call itself, on ZKsync Era, the necessary copying of return data happens only after the call has ended

Even though the data field is not used (see the empty space after the comma in (success,) below), it does receive this data and build it up in memory after the call has succeeded.

(success,) = to.call{value: value}(data);

Impact: Some calls that ought to return a fail (due to excessive build up of memory) will pass the initial success check, and only fail afterwards through an evm error. Or, inversely, because _executeTransaction allows functions to return data and have it stored in memory, some functions fail that ought to succeed.

The above especially applies to transactions that call a function that returns large amount of bytes.

Additionally,

  • _executeTransaction is very gas inefficient due to this issue.

  • As the execution fails with a evm error instead of a correct MondrianWallet2__ExecutionFailed error message, functionality of frontend apps might be impacted.

Proof of Concept:

  1. A contract has been deployed that returns a large amount of data.

  2. MondrianWallet2 calls this contract.

  3. The contract fails with an evm error instead of MondrianWallet2__ExecutionFailed.

After mitigating this issue (see the Recommended Mitigation section below)
4. No call fail with an evm error anymore.

Proof of Concept

Place the following code after the existing tests in ModrianWallet2Test.t.sol:

contract TargetContract {
uint256 public arrayStorage;
​
constructor() {}
function writeToArrayStorage(uint256 _value) external returns (uint256[] memory value) {
arrayStorage = _value;
​
uint256[] memory arr = new uint256[](_value);
return arr;
}
}

Place the following code in between the existing tests in ModrianWallet2Test.t.sol:

// You'll also need --system-mode=true to run this test
function testMemoryAndReturnData() public onlyZkSync {
TargetContract targetContract = new TargetContract();
vm.deal(address(mondrianWallet), 100);
address dest = address(targetContract);
uint256 value = 0;
uint256 inputValue;
​
// transaction 1
inputValue = 310_000;
bytes memory functionData1 = abi.encodeWithSelector(TargetContract.writeToArrayStorage.selector, inputValue, AMOUNT);
Transaction memory transaction1 = _createUnsignedTransaction(mondrianWallet.owner(), 113, dest, value, functionData1);
transaction1 = _signTransaction(transaction1);
​
// transaction 2
inputValue = 475_000;
bytes memory functionData2 = abi.encodeWithSelector(TargetContract.writeToArrayStorage.selector, inputValue, AMOUNT);
Transaction memory transaction2 = _createUnsignedTransaction(mondrianWallet.owner(), 113, dest, value, functionData2);
transaction2 = _signTransaction(transaction2);
​
vm.startPrank(ANVIL_DEFAULT_ACCOUNT);
// the first transaction fails because of an EVM error.
// this transaction will pass with the mitigations implemented (see above).
vm.expectRevert();
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction1);
​
// the second transaction fails because of an ExecutionFailed error.
// this transaction will also not pass with the mitigations implemented (see above).
vm.expectRevert();
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction2);
​
vm.stopPrank();
}

Recommended Mitigation: By disallowing functions to write return data to memory, this problem can be avoided. In short, replace the standard .call with an (assembly) call that restricts the return data to length 0.

- (success,) = to.call{value: value}(data);
+ assembly {
success := call(gas(), to, value, add(data, 0x20), mload(data), 0, 0)
}
Updates

Lead Judging Commences

bube Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Call works differently on ZKsync

Support

FAQs

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