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

Reentrancy Vulnerability in Reward Claim Mechanism

4rdiii

[H-03] Reentrancy Vulnerability in Reward Claim Mechanism

Summary

The current implementation of the claimAllRewards and claimSingleReward functions deletes the rewardsOwned array only after transferring Ether. This opens up the contract to a reentrancy attack, allowing malicious users to repeatedly call the claim functions and withdraw all funds from the contract.

Vulnerability Details

The functions fail to implement the Checks-Effects-Interactions (CEI) pattern, which is a best practice in Solidity to prevent reentrancy vulnerabilities. Specifically, the rewards are deleted after the Ether transfer, allowing attackers to reenter the contract and claim rewards multiple times.

Here are the vulnerable functions:

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");
​
// Vulnerability: Deletion happens after the transfer, enabling reentrancy
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");
​
// Vulnerability: Deletion happens after the transfer, enabling reentrancy
delete rewardsOwned[msg.sender][_index];
}

Since the array is only modified after transferring funds, a malicious user can reenter the contract within the transfer and claim additional rewards, potentially draining all funds.

Impact

An attacker could exploit this reentrancy vulnerability to repeatedly withdraw rewards, effectively draining all funds in the contract and leaving legitimate users without access to their rewards.

Tools Used

Manual Review

Recommendations

To mitigate this vulnerability, follow the Checks-Effects-Interactions (CEI) pattern by updating the state (i.e., deleting the reward) before transferring Ether. Here's a secure implementation:

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");
​
// Update state before interaction
delete rewardsOwned[msg.sender];
​
(bool success, ) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
}
​
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");
​
// Update state before interaction
delete rewardsOwned[msg.sender][_index];
​
(bool success, ) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
}

This modification ensures that the reward is deleted before any Ether transfer occurs, preventing reentrancy attacks.

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!