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

Missing `minted` value check while reducing the tier count may cause unexpected behaviours

demorextess

Summary

Missing minted value check while reducing the tier count may cause unexpected behaviours

Vulnerability Details

External caller have an ability upgrade DAO tiers configurations using updateDAOMembership()function. This function checks various parameters in order to validate the transaction is legit and execute it.

But in current version, it doesn't check the mintedvalues if the upgrade is reducing the count of the tiers.

Let say, we have 7 tiers in our DAO and external caller wants to reduce it to 6. In order to execute that transaction successfully, firstly we need to check whether there is a minted NFT on tier 7 because it will be gone after execution.

Our current implementation currently, update the mintedamounts for lower tier indexes and then it sets the new tier configuration to global state variables.

// Preserve minted values and adjust the length of dao.tiers
for (uint256 i = 0; i < tierConfigs.length; i++) {
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
}
}
​
// Reset and update the tiers array
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
require(tierConfigs[i].minted <= tierConfigs[i].amount, "Invalid tier config");
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
​
// updating the ceiling limit acc to new data
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
​
dao.noOfTiers = tierConfigs.length;

For safety, it should also check the high tier indexes. In realistic scenarios, external caller is trusted entity and he won't execute that kind of transaction but we still need to check that situation. Because it still can be happen without depending on the external caller ( such as re-org situation ).

PoC

Following test will simulate the scenario

it("Should revert while update DAO Membership with invalid conf", async function () {
​
// Initial state, we have 2 tier
await currencyManager.addCurrency(testERC20.address); // Assume addCurrency function exists in CurrencyManager
TierConfig = [{ price: 300, amount: 11, minted: 0, power: 12 }, { price: 150, amount: 11, minted: 0, power: 12 }];
const tierIndex = 1;
await membershipFactory.createNewDAOMembership(DAOConfig, TierConfig);
​
// 1 person join DAO with index 1 ( It can happen in re-org situation or maybe user submitted the transaction slightly earlier than the external caller )
await testERC20.mint(addr1.address, ethers.utils.parseEther("200"));
await testERC20.connect(addr1).approve(membershipFactory.address, TierConfig[tierIndex].price); // Assume approve function exists in CurrencyManager
await membershipFactory.connect(addr1).joinDAO(membershipERC1155.address, tierIndex);
​
// Update DAO, it removes index 1 ( It should revert )
const newTierConfig = [{ price: 150, amount: 10, minted: 0, power:4 }];
await expect(membershipFactory.updateDAOMembership("testdao.eth", newTierConfig)).to.be.reverted;
});

Impact

Medium - It will break core functionality of the one world implementation. The users who have higher tier indexes can't upgrade their membership because noOfTiers is updated after the configuration. Additionally, their NFT will hold invalid tier indexes.

Tools Used

Manual Review

Recommendations

Following check is needed for safety of execution.

+ if (tierConfigs.length < dao.tiers.length) {
+ for (uint i = tierConfigs.length; i < dao.tiers.length; i++){
+ require(dao.tiers[i].minted == 0, "Invalid tier config");
+ }
+ }
Updates

Lead Judging Commences

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

Support

FAQs

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