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

[H-1]: `MysteryBox::changeOwner` function should be using an `onlyOwner` modifier to prevent any address from changing the contract owner

awacs

Vulnerability Details

The MysteryBox::changeOwner function is marked as public but does not implement any access-security modifier, such as onlyOwner. This function enables any address to change the MysteryBox::owner storage variable, thus enabling anyone to tamper with the protocol and access functions intended only to be used by the contract owner.

The function is missing access control modifier to probihit users from misusing the protocol.

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

Impact

Any caller may change the contract owner to themselves and then use functions that are meant to be used only for the owner. This way, any user may withdraw the funds from the protocol since the MysteryBox::withdrawFunds checks, that the caller is the owner.

Attack vector / PoC:

  1. Owner deploys the contract to the chain and deployers address is saved in the MysteryBox::owner variable.

  2. Any user can call the MysteryBox::changeOwner function since it is marked as public and change the storage variable MysteryBox::owner to a new value

PoC:

Insert the code below to the TestMysteryBox.t.sol suite and run the test.

function test_anyoneCanBecomeAnOwner() public {
​
// Storing the deployers address
address originalOwner = mysteryBox.owner();
​
// any address may call the changeOwner function
vm.prank(user1);
mysteryBox.changeOwner(user1);
vm.stopPrank();
​
// retrieving new owner's address
address newOwner = mysteryBox.owner();
​
console.log('Original owner: ', originalOwner);
console.log('Txn caller: ', user1);
console.log('New owner: ', newOwner);
​
// the storage owner variable was changed
assert(originalOwner != newOwner);
}

Code output:
Original owner: , owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
Txn caller: , ECRecover: [0x0000000000000000000000000000000000000001]
New owner: , ECRecover: [0x0000000000000000000000000000000000000001]

Tools Used

Static analysis

Recommendations

Utilize either onlyOwner contract by OpenZeppelin, or implement own onlyOwner modifier that will be checking that the caller is a contract owner. The MysteryBox::changeOwner should be marked as onlyOwner.

A simple modifier may look similar to this:

error MysteryBox__NotTheOwner();
modifier onlyOwner() {
if ( msg.sender != owner ) {
revert MysteryBox__NotTheOwner();
}
_;
}
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.