Mystery Box

Summary

The transferReward function deletes a specific reward by index without reorganizing the rewards array, leading to potential gaps and zeroed entries in the rewardsOwned mapping. This can cause inefficiencies and confusion when managing or displaying rewards to users.

Vulnerability Details

In the MysteryBox.sol contract, the transferReward function deletes a reward at a specific index:

Using delete on a specific index sets the Reward struct at that position to its default values ("" for name and 0 for value) but does not remove the element from the array. This results in empty or zeroed entries within the rewardsOwned array, which can lead to inefficiencies and unintended behavior when iterating over rewards.

Impact

this can lead to confusion for users by introducing empty rewards in the array. It may also cause increased gas consumption due to handling unnecessary zeroed entries, particularly if users accumulate a large number of rewards with gaps.

PoC

After transferring a reward, the rewardsOwned array contains an empty entry:

When retrieving User1's rewards, an empty reward appears:

This can lead to confusion when displaying rewards to users, as they may encounter unexpected empty entries.

Tools Used

Manual code review

Recommendations

Implement a more efficient removal mechanism that maintains the integrity of the rewards array without leaving gaps. One common approach is to swap the reward to be deleted with the last element and then remove the last element, reducing the array length by one.

Revised transferReward function:

This approach ensures that the rewardsOwned array remains contiguous and free of empty entries, improving efficiency and reducing potential confusion.