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

When the owner calls the function `MondrianWallet2::renounceOwnership` any funds left in the contract are stuck forever.

7Cedars

When the owner calls the function MondrianWallet2::renounceOwnership any funds left in the contract are stuck forever.

Description: MondrianWallet2 inherits the function renounceOwnership from openZeppelin's OwnableUpgradeable. This function simply transfers ownership to address(0).

See OwnableUpgradeable.sol:

function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
}

As is noted in OwnableUpgradeable.sol:

NOTE: Renouncing ownership will leave the contract without an owner,
thereby disabling any functionality that is only available to the owner.

Making any kind of transaction depends on a signature of the owner. As such, no transactions are possible after the owner renounces their ownership. This includes transfer of funds out of the contract.

Impact: In the life cycle of an Abstracted Account, renouncing ownership is a likely final action. It is very easy to forget to transfer any remaining funds out of the contract before doing so, especially when doing so in an emergency. As such, it is quite likely that funds are left in the contract by accident.

Proof of Concept:

  1. A user deploys MondrianWallet2 and transfers ownership to their address.

  2. The user transfers funds into the MondrianWallet2 account.

  3. The user renounces ownership and forgets to retrieve funds.

  4. User's funds are now stuck in the account forever.

Proof of Concept

Place the following in ModrianWallet2Test.t.sol.

// Please note that you will also need --system-mode=true to run this test.
function testRenouncingOwnershipLeavesEthStuckInContract() public onlyZkSync {
vm.deal(address(mondrianWallet), AMOUNT);
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);
​
vm.prank(mondrianWallet.owner());
mondrianWallet.renounceOwnership();
​
vm.prank(ANVIL_DEFAULT_ACCOUNT);
vm.expectRevert(MondrianWallet2.MondrianWallet2__NotFromBootLoaderOrOwner.selector);
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
vm.prank(BOOTLOADER_FORMAL_ADDRESS);
bytes4 magic = mondrianWallet.validateTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
vm.assertEq(magic, bytes4(0));
}

Recommended Mitigation: One approach is to override OwnableUpgradeable::renounceOwnership function, adding a transfer of funds to the contract owner when renounceOwnership is called.

Note that it is probably best not to make renouncing ownership conditional on funds having been successfully transferred. In some cases it might be more important to immediately renounce ownership (for instance when keys of an account have been compromised) rather than retrieving all funds from the contract.

Add the following code to MondrianWallet2.sol:

+ function renounceOwnership() public override onlyOwner {
+ uint256 remainingFunds = address(this).balance;
+ owner().call{value: remainingFunds}("");
+ _transferOwnership(address(0));
+ }
Updates

Lead Judging Commences

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.