Mystery Box

First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: low
Valid

Unbounded Loops in claimAllRewards may lead to DOS

Summary

The claimAllRewards function in the MysteryBox contract contains an unbounded loop that iterates over all rewards owned by a user. As the number of rewards grows, this loop could potentially consume more gas than the block gas limit allows, leading to a Denial of Service (DoS) condition where users with many rewards are unable to claim them.

Vulnerability Details

The vulnerability is present in the claimAllRewards function of the MysteryBox contract:

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

The key issues are:

  1. Unbounded Loop: The function iterates over all rewards owned by the user without any upper limit. As users accumulate more rewards, the gas cost of this function increases linearly.

  2. Gas Limit Risk: If a user accumulates a large number of rewards, the gas required to process this loop could exceed the block gas limit (currently around 30 million gas on Ethereum mainnet), making it impossible to execute the function.

  3. State Changes After Loop: The contract state is only updated after the entire loop completes and the ETH transfer is made. This means that if the function reverts due to hitting the gas limit, no rewards can be claimed at all.

  4. No Partial Claiming: The function doesn't allow for partial claiming of rewards, which could mitigate the gas limit issue.

This design creates a potential DoS condition where users with many rewards might be unable to claim them, effectively locking their rewards in the contract.

Impact

The impact of this vulnerability is potentially severe:

  1. Denial of Service: Users who accumulate a large number of rewards may be completely unable to claim them. This could result in permanent loss of funds for these users, as their rewards become effectively locked in the contract.

  2. Financial Loss: If users are unable to claim their rewards, they lose the monetary value associated with those rewards. This could be significant, especially for users who have been active participants over a long period of time.

  3. Unfair Advantage: Users with fewer rewards will be able to claim successfully, while those with more rewards (who have likely been more active or successful in the game) are penalized. This creates an unfair dynamic that goes against the intended game mechanics.

  4. Contract Instability: As more users accumulate rewards over time, an increasing number of accounts may become "stuck", unable to interact with this part of the contract functionality. This could lead to a gradual degradation of the contract's usability.

The severity of this impact increases over time as users accumulate more rewards, making it a ticking time bomb for the contract's long-term viability and user satisfaction.

Tools Used

  • Manual review of the smart contract code

Recommendations

To address this vulnerability and significantly improve gas efficiency, we recommend redesigning the reward tracking system:

  1. Replace the array-based reward storage with a mapping-based system:

+ mapping(address => uint256) public totalRewardsValue;
+ mapping(address => mapping(string => uint256)) public rewardCounts;
  1. Update the reward addition logic:

+ function addReward(address user, string memory rewardName, uint256 value) internal {
+ totalRewardsValue[user] += value;
+ rewardCounts[user][rewardName]++;
}
  1. Modify the claimAllRewards function to use the new storage system:

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");
+ uint256 totalValue = totalRewardsValue[msg.sender];
+ require(totalValue > 0, "No rewards to claim");
delete rewardsOwned[msg.sender];
+ totalRewardsValue[msg.sender] = 0;
+ rewardCounts[user][rewardName] = 0;
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
}

This gas-efficient approach eliminates the need for looping through an array of rewards, significantly reducing gas costs and removing the potential for a DoS condition. It maintains a total value for quick claiming and individual counts for each reward type if detailed tracking is required. This design scales well regardless of the number of rewards a user accumulates.

Updates

Appeal created

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Gas Limit Exhaustion in `claimAllRewards` Function

Support

FAQs

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