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

Resetting Elements in `rewardsOwned` Array Instead of Removing Them Leads to Inefficiencies

afdholudin

Summary

The transferReward, claimSingleReward, and claimAllRewards functions in the MysteryBox contract reset specific elements in the rewardsOwned array to their default state instead of entirely removing them. This design introduces inefficiencies in reward management and can create confusion regarding the actual rewards available to users.

Vulnerability Details

In the transferReward, claimSingleReward, and claimAllRewards functions, the line that employs delete is intended to remove rewards associated with the msg.sender upon various actions, such as transferring or claiming rewards. However, this operation does not eliminate the element from the rewardsOwned array; instead, it resets that specific entry to its default state (an empty Reward struct). As a result, the length of the array remains unchanged, leading to "holes" within the array.

transferReward function:

function transferReward(address _to, uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
@> delete rewardsOwned[msg.sender][_index]; // This resets the index to default, not removing it
}

claimSingleReward function:

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

claimAllRewards function:

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

Instead of removing the elements from the rewardsOwned array, this line resets all entries for that user to their default state, leading to unnecessary empty slots. This causes inefficient gas usage when iterating through rewards in other functions.

As a result, the presence of these default values can produce unexpected behavior in various parts of the contract, especially in functions that rely on iterating through the rewardsOwned array and calculating available rewards. Other functions that process rewards may end up consuming gas on operations that yield no value, leading to wasted gas.

Impact

The presence of these "empty" rewards can confuse users when they check their available rewards, as they may see entries that are reset but not truly removed. Furthermore, the gas wastage caused by redundant calculations on empty entries can increase the cost of executing transaction functions. This inefficiency negatively impacts users by increasing gas consumption.

Tools Used

  • Manual review

Recommendations

One effective solution to this issue is to shift the rewards within the array when one is removed, or to adopt a dynamic management pattern that adjusts the array's length accordingly.

function transferReward(address _to, uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
- delete rewardsOwned[msg.sender][_index]; // This resets the index to default, not removing it
// Step 1: Replace the element at _index with the last element in the array
+ uint256 lastIndex = rewardsOwned[msg.sender].length - 1;
+ if (_index != lastIndex) { // Only swap and pop if we're not deleting the last element
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][lastIndex];
+ }
​
// Step 2: Remove the last element from the array (this effectively "shrinks" the array)
+ rewardsOwned[msg.sender].pop();
}
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];
​
+ uint256 lastIndex = rewardsOwned[msg.sender].length - 1;
+ if (_index != lastIndex) { // Only swap and pop if we're not deleting the last element
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][lastIndex];
+ }
​
+ rewardsOwned[msg.sender].pop();
}
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];
+ uint256 lastIndex = rewardsOwned[msg.sender].length - 1;
+ if (_index != lastIndex) { // Only swap and pop if we're not deleting the last element
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][lastIndex];
+ }
​
+ rewardsOwned[msg.sender].pop();
}
Updates

Appeal created

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

A user can poison the `rewardsOwned` of another user via `transferReward` of an empty reward index

Gas Limit Exhaustion in `claimAllRewards` Function

Support

FAQs

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

Give us feedback!