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

Using `delete` on the `rewardsOwned` mapping leads to inefficiencies as the mapping retains the empty space for the deleted entry

codedesignz

Description: There are three instances where delete operation is performed on rewardsOwned in MysteryBox contract. delete operation sets the element at that specified index to the default value for the Reward struct (in this case, an empty Reward struct with a name of an empty string and a value of 0). The mapping still retains its full length, and the element at _index will simply be an empty reward.

Impact: Having empty entries can lead to the below issues.

  1. Gaps in the Mapping, resulting in:

    Wasted space: The array still holds the empty element, which consumes memory and gas unnecessarily.
    Incorrect logic and gas inefficiency: The other parts of the contract (e.g., loops or functions) that iterate over the rewardsOwned array will encounter empty reward slots and may behave incorrectly. For instance, if a user has already claimed all of his boxes reward, calling MysterBox::claimAllRewards will still loop through all of his empty rewards and cost gas.

Proof of Concept:

  1. User A purchases 5 boxes by calling MysteryBox::buyBox function.

  2. He then opens these boxes by MysteryBox::openBox function.

  3. He either transfers some of the boxes to user B by using MysteryBox::transferReward function or claims the reward for himself by MysteryBox::claimSingleReward or MysteryBox::claimAllRewards.

  4. When he calles MysteryBox::getRewards function, he will still get 5 entries returned. Although the entries for the claimed/transferred reward should have been removed.

Proof of Code

Code

Place the folowing into TestMysteryBox.t.sol

function testTransferRewardDoesNotRemoveTheEntryFromMapping() public {
uint256 boxesToBuy = 5;
vm.startPrank(user1);
vm.deal(user1, boxesToBuy * 0.1 ether);
// Purchase multiple boxes and open them
for (uint8 i = 0; i < boxesToBuy; i++) {
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
}
​
MysteryBox.Reward[] memory userRewardsBeforeTransferringAReward = mysteryBox.getRewards();
// Check if the user has rewards
console2.log("Total rewards before transfer: ", userRewardsBeforeTransferringAReward.length);
for (uint8 i = 0; i < userRewardsBeforeTransferringAReward.length; i++) {
console2.log("Reward [", i, "] name: ", userRewardsBeforeTransferringAReward[i].name);
console2.log("Reward [", i, "] value: ", userRewardsBeforeTransferringAReward[i].value);
}
​
mysteryBox.transferReward(user2, 3);
MysteryBox.Reward[] memory userRewardsAfterTransferringAReward = mysteryBox.getRewards();
vm.stopPrank();
console2.log("Total rewards after transferring: ", userRewardsAfterTransferringAReward.length);
for (uint8 i = 0; i < userRewardsAfterTransferringAReward.length; i++) {
console2.log("Reward [", i, "] name: ", userRewardsAfterTransferringAReward[i].name);
console2.log("Reward [", i, "] value: ", userRewardsAfterTransferringAReward[i].value);
}
// The number of entries for the Rewards registered for user1 stay the same even after the reward is transferred.
assertEq(userRewardsBeforeTransferringAReward.length, userRewardsAfterTransferringAReward.length);
}

Recommended Mitigation: Use pop instead of delete wherever a single entry is supposed to be removed from the mapping.

Update MysteryBox::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];
+ // Replace the deleted reward with the last element to prevent gaps
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][
+ rewardsOwned[msg.sender].length - 1
+ ];
+ rewardsOwned[msg.sender].pop();
}

Update MysteryBox::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];
+ // Replace the deleted reward with the last element to prevent gaps
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][
+ rewardsOwned[msg.sender].length - 1
+ ];
+ 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!