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

[H-1] Missing Access Control Checks on `MysteryBox::changeOwner`

JJS

Summary

In the `MysteryBox.sol` contract many functions rely on a `require` that checks if the `msg.sender` is in fact the owner. These functions include:

- `setBoxPrice` - Setting the box price
- `addReward` - Adding a whole new reward
- `withdrawFunds` - Withdrawing the funds from the contract

All of these but not the `changeOwner` function, which changes the address of the owner!

Vulnerability Details

Missing access control checks on the `changeOwner` function which changes the address of the owner:

function changeOwner(address _newOwner) public {
//* Missing code here! *//
owner = _newOwner;
}

Impact

This vulnerability could potentially lead to loss of all funds in the contract. Consider the following scenario and test that can be added to testMysteryBox.t.sol:

1. The owner deployed with 0.1 ether (in the setup)
2. Two users buy boxes
3. An attacker sees the vulnerability
4. The attacker changes himself to be the owner
5. The attacker (now the owner of the contract) withdraws the funds

function test_can_steal_funds_as_owner() public {
uint256 amount = 0.1 ether;
vm.deal(user1, amount);
vm.deal(user2, amount);
vm.prank(user1);
mysteryBox.buyBox{value: amount}();
vm.prank(user2);
mysteryBox.buyBox{value: amount}();
address attacker = makeAddr("attacker");
vm.startPrank(attacker);
mysteryBox.changeOwner(attacker);
mysteryBox.withdrawFunds();
vm.stopPrank();
assertEq(attacker.balance, amount * 3); // 0.1 ether * 3 from user1, user2 and the owner the deployed with 0.1 as well. }

Tools Used

Manual Review, Unit Test

Recommendations

Add an onlyOwner modifier (then can be added to the necessary functions mentioned earlier):

+ modifier onlyOwner() {
+ if (msg.sender != owner){
+ revert MysteryBox__NotOwner;
+ }
+ _;
+ }

Or can just add a check in `changeOwner` for owner:

function changeOwner(address _newOwner) public {
+ require (msg.sender == owner, "Not Owner!");
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.