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

Underflow Issue in `upgradeTier` Function

0xbeastboy

Github

Summary

In the MembershipFactory contract's upgradeTier function, attempts to upgrade from tier 0 (the highest tier) result in silent arithmetic underflow reverts rather than meaningful error messages. While not a security vulnerability due to Solidity 0.8.x's built-in overflow protection, this creates a poor user experience and could complicate contract interaction.

Vulnerability Details

The vulnerability exists in the upgradeTier function where top-tier membership upgrades are handled:

function upgradeTier(address daoMembershipAddress, uint256 fromTierIndex) external {
require(daos[daoMembershipAddress].daoType == DAOType.SPONSORED, "Upgrade not allowed.");
require(daos[daoMembershipAddress].noOfTiers >= fromTierIndex + 1, "No higher tier available.");
IMembershipERC1155(daoMembershipAddress).burn(_msgSender(), fromTierIndex, 2);
IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), fromTierIndex - 1, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, fromTierIndex - 1);
}

First, the tier system uses an inverse numbering scheme where tier 0 represents the highest tier level. While this is a common pattern, the function's error handling doesn't explicitly account for this design choice in its validation steps.

Second, while the function includes a check to ensure a higher tier exists (noOfTiers >= fromTierIndex + 1), it doesn't explicitly validate whether the current tier is upgradeable. Instead, it relies on Solidity's arithmetic underflow protection when attempting to calculate the target tier (fromTierIndex - 1).

Third, when a member at tier 0 attempts to upgrade, rather than receiving a clear error message explaining that they're already at the highest tier, they receive a generic panic error due to arithmetic underflow. This creates confusion and a poor user experience, especially for contract integrations that may not handle panic errors gracefully.

Proof of Concept:

// Test Case demonstrating the issue
function testTopTierUpgradeFailure() public {
// Setup: Create DAO with multiple tiers
setupSponsoredDAO();
// Join highest tier (tier 0)
joinDAO(daoAddress, 0);
// Attempt to upgrade from tier 0
// This will revert with arithmetic underflow
try upgradeTier(daoAddress, 0) {
fail("Should revert");
} catch Error(string memory reason) {
// Won't catch here because it's a panic, not a require revert
fail("Should not reach here");
} catch Panic(uint /* code */) {
// Will catch here with panic code 0x11 (arithmetic underflow)
// Success - but this is not user friendly
}
}

Impact

Members attempting to upgrade from the highest tier receive cryptic panic errors instead of clear explanations. This creates confusion and may lead to support issues. External contracts or dApps interacting with the membership system need special handling for panic errors, which are less standard than require-based reverts. This could lead to integration challenges or broken user interfaces.

Tools Used

Manual Review

Recommendations

Ensure that the index is greater than 0:

// Add explicit check for highest tier
require(fromTierIndex > 0, "Already at highest tier");
Updates

Lead Judging Commences

0xbrivan2 Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
0xbrivan2 Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

0xbeastboy Submitter
7 months ago
0xbrivan2 Lead Judge
6 months ago
0xbrivan2 Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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