Mystery Box

Summary

The MysteryBox contract uses the delete operation to remove rewards from the rewardsOwned mapping, which is stored as an array inside a struct. While the delete keyword resets the entry to its default value (an empty reward), it does not remove the slot or shift the array elements. This leads to inefficiencies due to empty slots left in the array, which increases gas costs and causes potential confusion when users or external systems interact with the rewards array.

Vulnerability Details

The rewardsOwned mapping is an array structure where user rewards are stored (mapping(address => Reward[]) public rewardsOwned). Functions such as transferReward and claimSingleReward delete entries from this mapping using the delete keyword:

The delete keyword resets the reward entry at the specified index but does not remove the slot, leaving empty entries (default Reward("", 0)) in the array. These gaps cause inefficiencies during iterations and can confuse users when they view or interact with their rewards.

For example, in the claimAllRewards() function, the array is looped through to sum the values of all rewards:

These empty slots remain in the array after deletion, leading to increased gas costs and inefficiencies when looping through the array. This behavior could also cause confusion for users who expect a contiguous array of rewards, as empty or default values may remain in the array.

Impact

• Increased Gas Costs: Iterating through arrays with empty slots leads to higher gas costs, as the contract unnecessarily processes default values.

• Potential Confusion: Users or external systems may be confused by the presence of empty slots when viewing or interacting with the rewards, leading to unexpected behavior.

• Unexpected Behavior: If the contract expects a contiguous array, these gaps can cause problems in functionality, especially in functions that rely on iterating over all rewards.

Tools Used

Manual code review

Recommendations

• Shift Array Elements After Deletion:
  When deleting an entry from the rewards array, shift the remaining elements to fill the gap. This ensures that the array remains contiguous and does not contain empty slots.

  Example:

• Use Mappings with Unique Keys Instead of Arrays:
  Consider using mappings with unique keys rather than arrays to manage user rewards. Mappings allow for better flexibility and prevent gaps, as each reward can be accessed and removed individually without affecting the overall structure.

• Implement a Marker for Removed Entries:
  Instead of using the delete keyword, consider marking rewards as “inactive” or “claimed,” and filter out these entries during iterations. This maintains clarity over all rewards while avoiding the creation of gaps.

By implementing these changes, the contract will reduce gas costs, improve efficiency, and prevent confusion, ensuring smoother user interactions and cleaner array management.