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

Lack of owner access control in `MondrianWallet2::_authorizeUpgrade` poses security risk

soupy

Summary

MondrianWallet2::_authorizeUpgrade is missing onlyOwner access control to restrict upgrade implementation can only be done by owner

Vulnerability Details

The function MondrianWallet2::_authorizeUpgrade currently lacks an implementation to enforce that only the contract owner can call it. This oversight poses a significant risk during upgrades, potentially allowing non-owners to execute the upgrade process. That could introduce malicious functionalities in the new implementation, such as unauthorized fund transfers or actions that damage the owner's reputation or credibility.

Proof of Concept :

In the file test/ModrianWallet2Test.t.sol, add the following test :

​
function testContractUpgradabilityByRandomUser() public {
MondrianWallet2 mockNewImplementation = new MondrianWallet2();
address randomUser = makeAddr("RandomUser");
​
vm.prank(randomUser);
mondrianWallet.upgradeToAndCall(address(mockNewImplementation), "");
​
}
​

The test will pass indicating that any random user can upgrade the contract. However, the same test will fail if we add onlyOwner modifier to MondrianWallet2::_authorizeUpgrade as outlined in the recommendation section below.

Impact

New implementation can be done by non-owner resulting potential malicious functions/activities to be included in the upgraded contract.

Tools Used

Manual review

Recommendations

It's crucial to implement proper access control mechanisms to restrict the contract upgrades. Recommend to include onlyOwner modifier to MondrianWallet2::_authorizeUpgrade :

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

Lead Judging Commences

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