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

Missing `MondrianWallet2::receive` and `MondrianWallet2::fallback` functions make it impossible to move funds into the contract.

7Cedars

Missing MondrianWallet2::receive and MondrianWallet2::fallback functions make it impossible to move funds into the contract. Combined with the absence of a Paymaster account, it means it is impossible to validate transactions, breaking core functionality of the Account Abstraction.

Description: MondrianWallet2.sol is missing a receive and fallback function. It makes it impossible to move funds into the contract.

constructor() {
_disableInitializers();
}
​
@> // receive and / or fallback function should be here.
​
function validateTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction)

Impact: Because MondrianWallet2.sol does not set up a paymaster account, the Account Abstraction will only work if MondrianWallet2.sol itself has sufficient balance to execute transactions. If not, the MondrianWallet2::validateTransaction will fail and return bytes4(0).

Lacking a receive and fallback function, it is impossible to move funds into the contract: Any empty call with ether will revert and calls to a function will return excess ether to the caller, leaving no funds in the contract.

This, in turn, means that the function MondrianWallet2::_validateTransaction will always revert:

uint256 totalRequiredBalance = _transaction.totalRequiredBalance();
// this conditional will always return false.
if (totalRequiredBalance > address(this).balance) {
revert MondrianWallet2__NotEnoughBalance();
}

The only way to execute a transaction is by the owner of the contract through the MondrianWallet2::executeTransaction, which has the owner pay for the transaction directly. This approach of executing a transaction is exactly the same as the owner themselves executing the transaction directly, rendering the Account Abstraction meaningless.

An additional note on testing. This issue did not emerge in testing because the account is added ether through a cheat code in MondrianWallet2Test.t.sol::setup:

vm.deal(address(mondrianWallet), AMOUNT);

Although common practice, it makes issues within funding contracts easy to miss.

Proof of Concept:

  1. User deploys an account abstraction and transfers ownership to themselves.

  2. User attempts to transfer funds to the contract and fails.

  3. Bootloader attempts to validate transaction, fails.

  4. User attempts to execute transaction directly through MondrianWallet2::executeTransaction and succeeds.

In short, the only way transactions can be executed are directly by the owner of the contract, defeating the purpose of Account Abstraction.

Proof of Concept

First remove cheat code that adds funds to mondrianWallet account in ModrianWallet2Test.t.sol::setup [sic: note the missing n!].

- vm.deal(address(mondrianWallet), AMOUNT);

And set the proxy to payable:

- mondrianWallet = MondrianWallet2(address(proxy));
+ mondrianWallet = MondrianWallet2(payable(address(proxy)));

Then add the following to ModrianWallet2Test.t.sol.

// Please note that you will also need --system-mode=true to run this test.
function testMissingReceiveBreaksContract() public onlyZkSync {
// setting up accounts
uint256 AMOUNT_TO_SEND = type(uint128).max;
address THIRD_PARTY_ACCOUNT = makeAddr("3rdParty");
vm.deal(THIRD_PARTY_ACCOUNT, AMOUNT);
// Check if mondrianWallet indeed has no balance.
assertEq(address(mondrianWallet).balance, 0);
// create transaction
address dest = address(usdc);
uint256 value = 0;
bytes memory functionData = abi.encodeWithSelector(ERC20Mock.mint.selector, address(mondrianWallet), AMOUNT);
Transaction memory transaction = _createUnsignedTransaction(mondrianWallet.owner(), 113, dest, value, functionData);
transaction = _signTransaction(transaction);
​
// Act & assert
// sending money directly to contract fails; it leaves contract with balance of 0.
vm.prank(mondrianWallet.owner());
(bool success, ) = address(mondrianWallet).call{value: AMOUNT_TO_SEND}("");
assertEq(success, false);
assertEq(address(mondrianWallet).balance, 0);
​
// as a result, validating transaction by bootloader fails
vm.prank(BOOTLOADER_FORMAL_ADDRESS);
vm.expectRevert();
mondrianWallet.validateTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
​
// the same goes for executeTransactionFromOutside
vm.prank(THIRD_PARTY_ACCOUNT);
vm.expectRevert();
mondrianWallet.executeTransactionFromOutside(transaction);
​
// also when eth is send with the transaction.
vm.prank(THIRD_PARTY_ACCOUNT);
vm.expectRevert();
mondrianWallet.executeTransactionFromOutside{value: AMOUNT_TO_SEND}(transaction);
​
// It is possible to execute function calls by owner through execute Transaction. But this defeats the purpose of Account Abstraction.
vm.prank(mondrianWallet.owner());
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
assertEq(usdc.balanceOf(address(mondrianWallet)), AMOUNT);
}

Recommended Mitigation:
Add a payable fallback function to the contract.

+ fallback() external payable {
// an additional check is needed so that the bootloader will never end up calling the fallback function.
+ assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS);
+ }
Updates

Lead Judging Commences

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

Missing receive function

Support

FAQs

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