Mystery Box

Summary

The changeOwner() function lacks proper access control, allowing any user to change the contract's owner, posing a critical security risk.

Vulnerability Details (POC)

Affected Code:

The function is declared as public and lacks a require statement to restrict its usage to the current owner. As a result, any user can call changeOwner() and set themselves or another address as the new owner.

Scenario

An attacker notices the lack of access control and decides to exploit it:

1. Attacker's Action:

   • Calls changeOwner() with their own address.

     mysteryBox.changeOwner(attackerAddress);

2. Result:

   • owner is now set to attackerAddress.

   • The attacker gains full control over the contract.

3. Potential Exploits by Attacker:

   • Withdraw all funds using withdrawFunds().

   • Change the boxPrice to an exorbitant amount or to zero.

   • Add or remove rewards arbitrarily.

Reference to Test Code

In the provided test code, the test testChangeOwner_AccessControl() unintentionally demonstrates this vulnerability:

This test shows that user1, who is not the original owner, can successfully change the owner to themselves.

Impact

• Loss of Control: The legitimate owner loses control over the contract.

• Financial Loss: The attacker can withdraw all Ether from the contract.

• User Trust: Users may lose trust if the contract is compromised.

• Service Disruption: The attacker could halt or alter contract functionality.

Tools Used

• Manual Code Review: Identified the absence of access control in changeOwner().

• Testing Framework (Foundry): Demonstrated the vulnerability through a test case.

Recommendations

Add a require statement to ensure only the current owner can change ownership:

Modify the test to expect a revert when a non-owner tries to change ownership: