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

Missing minted value check in `updateDAOMembership()` may cause `amount < minted` inequality

demorextess

Summary

Missing minted value check in updateDAOMembership() may cause amount < minted inequality

Vulnerability Details

updateDAOMembership()function is used for updating the tier configurations of the DAO. It considers the current minted amounts while updating the DAO.

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

Our new tier config is updated with current dao's tier's minted values. Later, we're removing the all the items from old dao tiers and we're pushing our updated tier config

// Reset and update the tiers array
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount; // @audit it should check the current minted value if the minted value is higher than the new amount it should revert
// This situation will cause insolvency
}

In here, we're using the new tier config amounts for update and it will overwrite the old amounts. The vulnerability arise here because if our new amount value is lower than the previous amount value, we should also check the already minted value is lower than the new amount value. The amountvalue represents the maximum mintable NFT amount of that tier and the following inequality should be always true:

External caller is trusted in here and we expect he will never pass an incorrect amount value here but we still need that check because in chain re-org or front-running the transaction attack vectors, we can't guarantee that situation won't happen.

Possible Scenario:

  1. In tier index 0, the amountvalue is 11 and the minted value is 10. There is 1 available slot for tier index 0.

  2. External caller wants to update the amountto 10.

  3. Attacker sees the external caller's transaction and he called joinDAOwith tier index 0. ( With frontrun )

  4. Now the protocol will mint 1 NFT to attacker and it will update the minted value to 11.

  5. Then, external caller's transaction is executed and amount is 10 right now.

  6. The inequality will be false.

PoC

Following test should be failed normally but it doesn't fail.

it("Should revert while updating DAO Membership", async function () {
​
// Initial state 10 minted at tier 0 index and 11 max amount
await currencyManager.addCurrency(testERC20.address); // Assume addCurrency function exists in CurrencyManager
TierConfig = [{ price: 300, amount: 11, minted: 0, power: 12 }];
const tierIndex = 0;
await membershipFactory.createNewDAOMembership(DAOConfig, TierConfig);
for (let i = 0; i < 10; i++) {
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);
}
// Frontrun or re-org happened
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 should revert normally )
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 the core system functionality and it will cause minted > amount. Additionally, it will bypass the max member limit because max member always holds the summation of all the tier amounts.

Tools Used

Manual Review

Recommendations

// 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;
}
Updates

Lead Judging Commences

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

Support

FAQs

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