Mystery Box

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

Lack of Access Control in `changeOwner()` Function

Summary

The changeOwner() function in the MysteryBox contract lacks proper access control, allowing any address to become the owner of the contract. This critical vulnerability can lead to a complete takeover of the contract and loss of funds.

Vulnerability Details

The changeOwner() function is implemented as follows:

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

This function is public, meaning it can be called by any external address. There are no checks to ensure that only the current owner can call this function. As a result, any address can call this function and set themselves (or any other address) as the new owner of the contract.

Impact

The impact of this vulnerability is severe:

  1. Unauthorized Contract Takeover: Any malicious actor can become the owner of the contract at will.

  2. Fund Theft: The new owner can call withdrawFunds() to steal all the ETH stored in the contract.

  3. Price Manipulation: The new owner can manipulate the box price using setBoxPrice(), potentially draining users' funds.

  4. Reward Manipulation: The new owner can add or modify rewards, potentially creating worthless rewards or removing valuable ones.

Given that the owner has complete control over the contract's crucial functions, this vulnerability effectively allows for a complete compromise of the contract's integrity and user funds.

Tools Used

This vulnerability was discovered through manual code review. No specific tools were required to identify this issue.

Recommendations

To fix this vulnerability, implement proper access control for the changeOwner() function. Here's a recommended implementation:

function changeOwner(address _newOwner) public {
require(msg.sender == owner, "Only the current owner can change ownership");
require(_newOwner != address(0), "New owner cannot be the zero address");
owner = _newOwner;
emit OwnershipTransferred(owner, _newOwner);
}

This implementation:

  1. Ensures only the current owner can change ownership.

  2. Prevents setting the owner to the zero address.

  3. Emits an event for transparency (you'll need to define this event).

Additionally, consider implementing a two-step ownership transfer process for added security, where the new owner must accept ownership in a separate transaction.

Updates

Appeal created

inallhonesty Lead Judge 11 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.