Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Hard-coding the number of user tokens in the `MembershipERC1155::burnBatch` function can lead to some burns being missed or unnecessary checks being made.

0xrolko

Relevant GitHub Links

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L78

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L81

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L96

Summary

In the MembershipERC1155::burnBatch function, the number of tokens to burn is hard-coded to 7, however this function is supposed to burn all the tokens owned by a user. So what happens if the user has less or more than this fixed number?

Vulnerability Details

The purpose of the MembershipERC1155::burnBatch function is to burn all the tokens of a single user, however the number of tokens this function have to burn is set to 7 as implemented in the :

@> /// @notice Burn all tokens of a single user
/// @param from The address from which tokens will be burned
function burnBatch(address from) public onlyRole(OWP_FACTORY_ROLE) {
// @audit Hardcoded number in the loop like this is risky
@> for (uint256 i = 0; i < 7; ++i) {
uint256 amount = balanceOf(from, i);
if (amount > 0) {
burn_(from, i, amount);
}
}
}

So, even if the user has less than 7 to burn, the for loop will continue to run until they reach that fixed number. On the other hand, if the user has more than 7 to burn, only 7 will be burned, the rest will remain in the contract and the function will have to be called again to burn more 7 until all the user's tokens are burned. The same logic is implemented in the MembershipERC1155::burnBatchMultiple function:

function burnBatchMultiple(address[] memory froms)
public
onlyRole(OWP_FACTORY_ROLE)
{
for(uint256 j = 0; j < froms.length; ++j){
// @audit Hardcoded number in the loop like this is risky
@> for(uint256 i = 0; i < 7; ++i){
uint256 amount = balanceOf(froms[j], i);
if (amount > 0) {
burn_(froms[j], i, amount);
}
}
}
}

Impact

unburnt token after the 7 loop, if the user has more than 7 tokens. And unnecessary check if the user has less than 7 tokens.

Tools Used

Manual review.

Recommendations

Instead of hard-coding 7, determine the actual number of tokens owned by the user dynamically, using mapping or other logic.
I personally recommend this approach:

mapping(address => mapping(uint256 => bool)) private _userHasToken;
mapping(address => uint256) private _userTokenCount;

  • The first mapping _userHasToken tracks whether the user holds a particular tokenId when mining.

  • And the second mapping _userTokenCount tracks the number of distinct tokens each user holds. This is the mapping that can be used instead of 7 when looping through the for loop.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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