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

Missing validation on the total amount of nft authorized versus the amount of minted one on `updateDAOMembership` could block new users from joining the DAO

befree3x

Summary

Updating DAO membership without validating the total amount of nft authorized for a tier with the amount of minted nfts can sometimes prevent new users from joining the DAO

Vulnerability Details

MembershipFactory::joinDAO always check that a user can not join a DAO at a specific tier if the amount of minted nfts exceeds the total amount authorized. However, such validation is not enforced when updating the DAO membership with MembershipFactory::updateDAOMembership.

As described below, when updating the tier configuration for a specific DAO, the protocol preserved well the minted values.

function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
....
// 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++) {
dao.tiers.push(tierConfigs[i]); // @audit tierConfigs[i].amount can be smaller than tierConfigs[i].minted, leading to unexpected behaviors
maxMembers += tierConfigs[i].amount;
}
....

On the other hand, it then stores directly the tier configuration tierConfigs[i] inside the dao.tiers array without making sure that the value of tierConfigs[i].amount must be bigger than the amount of nfts already minted dao.tiers[i].minted.

Impact

When an incorrect tier configuration above happens to a DAO, the amount of minted nfts will be superior than the total amount of nfts authorized, blocking new users from joining the DAO.

Proof of Concept

The following test can be added inside the main suite describe("MembershipFactory") in MembershipFactory.test.ts.
Below are the step to reproduce the PoC:

  • Define this new variable newAddrs just after this line let DAOType:any, DAOConfig:any, TierConfig:any;

let DAOType:any, DAOConfig:any, TierConfig:any;
let newAddrs:any;
​
​

  • Then, add this test inside the main suite describe("MembershipFactory")

​
​
describe("Join DAO can fail after an incorect DAO membership update", function () {
beforeEach(async function() {
await currencyManager.addCurrency(testERC20.address); // Assume addCurrency function exists in CurrencyManager
await membershipFactory.createNewDAOMembership(DAOConfig, TierConfig);
const ensAddress = await membershipFactory.getENSAddress("testdao.eth");
membershipERC1155 = await MembershipERC1155.attach(ensAddress);
newAddrs = await ethers.getSigners();
});
​
​
it("Should block new users from joining if updating DAO Membership with incorrect amount on a tier index", async function () {
const tierIndex = 1;
​
​
// In current TierConfig, for tier index 1, 10 users can join at maximum
// Supposed that 9 users have joined the DAO
for (var i = 0; i < 9; i++) {
await testERC20.mint(newAddrs[i].address, ethers.utils.parseEther("200"));
await testERC20.connect(newAddrs[i]).approve(membershipFactory.address, TierConfig[tierIndex].price); // Assume approve function exists in CurrencyManager
// Each user joins the dao normally
// There are in total 9 users joined, where the current max members is 10, one more user should be able to join
await membershipFactory.connect(newAddrs[i]).joinDAO(membershipERC1155.address, tierIndex);
}
// Update the DAO to use the new tier configuration
// We should expect that other users should be able to join the dao in tier 1 again
// Notice the tier amount for tier index 1 is now 8, which is smaller than the current minted nfts for this tier
const newTierConfig = [{ price: 300, amount: 20, minted: 0, power: 12 }, { price: 200, amount: 8, minted: 0, power:6 }, { price: 100, amount: 30, minted: 0, power: 3 }];
// This update should not be possible, however, it passed successfully
await expect(membershipFactory.updateDAOMembership("testdao.eth", newTierConfig)).to.not.be.reverted;
// After this update, for tier index 1, we can see that the amount of nfts minted is bigger than the authorized amount
const tiersdata = await membershipFactory.tiers(membershipERC1155.address);
expect(tiersdata[1].amount < tiersdata[1].minted).to.be.true; // this check should be false instead of true
​
​
// Now, a new user trying to join the DAO but he'll fail to do so
await testERC20.mint(newAddrs[9].address, ethers.utils.parseEther("200"));
await expect(membershipFactory.connect(newAddrs[9]).joinDAO(membershipERC1155.address, tierIndex)).to.be.reverted;
})
});
​
​

  • Finally, start anvil in a terminal then run the test case in another terminal with npx hardhat test --grep "can fail after" --network localhost.

​
​
MembershipFactory
Join DAO can fail after an incorect DAO membership update
✔ Should block new users from joining if updating DAO Membership with incorrect amount on a tier index (247ms, 2286189 gas)
​
​
​
​
1 passing (2s)
​
​

Tools Used

Manual review.

Recommendations

It's recommended to check the value of tierConfigs[i].amount in comparison with the minted nft amount.

The following diff can be applied to the updateDAOMembership function:

​
​
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
+ if (tierConfigs[i].amount < dao.tiers[i].amount) {
+ tierConfigs[i].amount = dao.tiers[i].amount;
+ }
}
​
​
Updates

Lead Judging Commences

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

Appeal created

befree3x Submitter
11 months ago
0xbrivan2 Lead Judge
11 months ago
0xbrivan2 Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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