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

DoS vulnerability through array bloating and reward management inconsistencies due to array element deletion flaw in `Mysterybox::transferReward`

Hoover

Summary

The MysteryBox.sol contract contains a critical vulnerability in the MysteryBox::transferReward function. This vulnerability creates gaps in the reward array when transferring rewards, potentially leading to inconsistencies in reward management and unexpected behavior in other functions that rely on the integrity of the rewards array and potential denial-of-service attack by filling the array with empty slots.

Vulnerability Details

Affected code - https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L73-L77

The vulnerability stems from the MysteryBox::transferReward function's improper handling of array elements:

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

The delete operation resets the array element to its default value (empty string for name and 0 for value) but does not remove the element from the array or reduce its length. This creates a gap in the array, which can lead to inconsistencies and potential issues in other functions that interact with the rewards array.

PoC

The provided PoC demonstrates this vulnerability:

function testTransferReward_LogicFlaw() public {
// ... (setup code)
​
// Transfer the reward
vm.prank(user1);
mysteryBox.transferReward(user2, 0);
console.log("Reward transferred from user1 to user2");
​
// Verify user1's rewards array
vm.prank(user1);
MysteryBox.Reward[] memory user1RewardsAfterTransfer = mysteryBox.getRewards();
​
// Assertions to formally verify the vulnerability
assertEq(user1RewardsAfterTransfer.length, rewardsBeforeTransfer.length, "User1's reward array length should not change");
assertEq(user1RewardsAfterTransfer[0].name, "", "Deleted reward name should be empty");
assertEq(user1RewardsAfterTransfer[0].value, 0, "Deleted reward value should be zero");
}

This PoC demonstrates that after transferring a reward:

  1. The length of user1's reward array remains unchanged.

  2. The "deleted" reward is reset to default values (empty string for name and 0 for value) instead of being removed from the array.

The console output shows:

Reward Pool Length: 4
User1 rewards before transfer:
Number of rewards: 1
Reward name: Coal
Reward value: 0
Reward transferred from user1 to user2
User2 rewards after transfer:
Number of rewards: 1
Reward name: Coal
Reward value: 0
User1 rewards after transfer:
Number of rewards: 1
Reward name:
Reward value: 0

Impact

The vulnerability in the MysteryBox::transferReward function can lead to several issues:

  1. Inconsistent Reward Management: Functions that iterate over or access the rewards array may encounter unexpected behavior due to these "empty" slots.

  2. Potential for Exploitation: Malicious users could potentially exploit this behavior to manipulate the reward system or cause denial-of-service by filling the array with empty slots.

  3. Increased Gas Costs: As the array length doesn't decrease, users may incur unnecessary gas costs when interacting with functions that process the entire rewards array.

  4. Data Integrity Issues: The presence of these "ghost" rewards could lead to incorrect calculations or representations of a user's reward inventory.

  5. User Experience Degradation: Users may see inconsistent or confusing information about their rewards, leading to a poor user experience and potential loss of trust in the platform.

This issue can significantly impact the contract's functionality and reliability over time, especially as users accumulate and transfer more rewards.

Tools Used

Manual Review and Foundry Test

Recommendations

To address this vulnerability, consider implementing the following changes:

  1. Use array manipulation techniques to properly remove the transferred reward:

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];
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][rewardsOwned[msg.sender].length - 1];
+ rewardsOwned[msg.sender].pop();
}

  1. Alternatively, consider using a different data structure, such as a mapping with a separate array for indices, to manage rewards more efficiently:

mapping(address => mapping(uint256 => Reward)) private userRewards;
mapping(address => uint256[]) private userRewardIndices;

  1. Implement additional checks to ensure the integrity of the rewards array:

function transferReward(address _to, uint256 _index) public {
require(_index < userRewardIndices[msg.sender].length, "Invalid index");
uint256 rewardId = userRewardIndices[msg.sender][_index];
Reward memory reward = userRewards[msg.sender][rewardId];
require(reward.value > 0, "Invalid reward");
​
userRewards[_to][rewardId] = reward;
delete userRewards[msg.sender][rewardId];
​
// Remove the index from sender's array
userRewardIndices[msg.sender][_index] = userRewardIndices[msg.sender][userRewardIndices[msg.sender].length - 1];
userRewardIndices[msg.sender].pop();
​
// Add the index to recipient's array
userRewardIndices[_to].push(rewardId);
}

  1. Add events to log reward transfers for better transparency and easier tracking:

event RewardTransferred(address indexed from, address indexed to, uint256 indexed rewardId, string name, uint256 value);
​
function transferReward(address _to, uint256 _index) public {
// ... (implementation as above)
emit RewardTransferred(msg.sender, _to, rewardId, reward.name, reward.value);
}

By implementing these recommendations, the MysteryBox::transferReward function will properly manage the rewards array, preventing gaps and ensuring consistent behavior across the contract. This will significantly improve the reliability and efficiency of the reward system, enhancing both security and user experience.

Updates

Appeal created

inallhonesty Lead Judge 11 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.