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

Denial of Service (DoS) Vulnerability Due to Inefficient Array Management in claimSingleReward and claimAllRewards Functions

oxdvoe

Summary

The "claimAllRewards" functions in the MysteryBox contract have a problem that could make them very expensive to use. Eventually, if a user has collected many rewards, the function will stop working. The core issue is the inefficient handling of dynamic arrays and the use of the delete keyword to remove rewards.

Vulnerability Details

  • When the 'delete' keyword is used to remove an element from the array, it only removes the element of a particular index, and the size of the array remains the same; this leads to a problem where the dynamic array keeps growing larger and larger

  • In the claimAllRewards function, the contract loops through all the rewards a user has collected, which can become expensive if the list is long. The delete operation used to clear all rewards is also costly, further increasing the gas required. If the list of rewards grows large, this function could fail due to running out of gas.

Impact

If a user collects many rewards, both functions could stop working because they require too much gas, leading to a Denial of Service (DoS) situation. This means users would not be able to claim their rewards.

Steps to Reproduce

  1. Let a user collect a large number of rewards.

  2. Try to call either the claimSingleReward or claimAllRewards function.

  3. You will Notice the gas costs become very high, and eventually, the transaction fails because it runs out of gas.

function testDOS_ClaimAllRewards() public {
// Give user1 some ether to buy multiple boxes.
vm.deal(user1, 10 ether);
​
uint256 numberOfBoxes = 100;
uint256 gasUsed;
uint256 gasStart;
​
for (uint256 i = 0; i < numberOfBoxes; i++) {
// Simulate user1 buying and opening a box.
vm.startPrank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox(); // User opens the box to receive rewards.
​
// Measure gas cost for claiming rewards as they accumulate.
gasStart = gasleft();
​
// Get all rewards owned by user1 using the getRewards() function
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
​
for (uint256 j = 0; j < rewards.length; j++) {
if(rewards[j].value > 0) {
console.log("Reward Value:", rewards[j].value); // This is how we access the value of each reward
mysteryBox.claimAllRewards(); // Claim all rewards
gasUsed = gasStart - gasleft();
​
console.log("Claimed BOXES", i + 1);
console.log("Gas used after claimed Boxes", gasUsed);
}
}
vm.stopPrank();
}
}

Recommendations

1. Claiming Rewards in Smaller Steps: In claimAllRewards, instead of claiming all rewards in one transaction, allow users to claim their rewards over multiple transactions. This will spread out the gas costs.

2. Better Way to Remove Rewards: Instead of using delete in both functions, use a more efficient method. One way is to replace the reward you want to remove with the last reward in the list, and then remove the last reward using pop(). This is much cheaper.

Example:

function removeReward(address user, uint256 index) internal {
uint256 length = rewardsOwned[user].length;
require(index < length, "Invalid index");
​
// Swap the last reward into the deleted spot, then pop the last element
rewardsOwned[user][index] = rewardsOwned[user][length - 1];
rewardsOwned[user].pop();
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year 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.