Description: MysteryBox::claimAllRewards
function doesn't follow CEI {Checks, Effects and Impacts} and as a result enables participants to drain the contract balance.
In the MysteryBox::claimAllRewards
function, the rewardsOwned
mapping is updated after the msg.sender
has claimed the reward. This means if the msg.sender
is a contract with a fallback
or recieve
function, they can re call the claimAllRewards
function again and again. This will drain the all funds from the MysteryBox
contract.
Impact:
A malicious contract could exploit the lack of reentrancy protection, calling the MysteryBox::claimAllRewards
function multiple times before the balance is updated, draining the contract's funds.
Proof of Concept:
Attacker sets up a contract with fallback function calls the MysteryBox::claimAllRewards
function
The attacker contract first buy some boxes and open them with MysteryBox::buyBox
and MysteryBox::openBox
functions.
The attacker contract calls MysteryBox::claimAllRewards
function draining the contract balance.
Proof of Code:
place the following in the MysteryBox.t.sol::MysteryBoxTest
And this Attack contract as well
Tools Used:
Manual Review
Recommended Mitigations:
To prevent this we should have the MysteryBox::claimAllRewards
function update the rewardsOwned
mapping before making external call.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.