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

`MembershipFactory::updateDAOMembership` Erases Accounting for Minted Tiers

izuman

Summary

When the admin removes a tier from DAO, then the tier is added back the minted amount will be zero, but the actual minted tokens will remain.

Vulnerability Details

Whenever a updateDAOMembership is called here: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/MembershipFactory.sol#L100

The tiers configs can be modified and removed.
When a tier is removed and then later added back at a later date the minted amount will be reset it the MembershipFactory while the MembershipERC1155 will remain.

As a result, the accounting will be different between contracts.

The TierConfig.amount which limits the number of tokens for each tier will no longer be relevant.

PoC

Paste the following test into the MembershipFactory.test.ts , run anvil and then run the test with the following command npx hardhat test --grep "Erase MembershipFactory account for tiers minted" --network localhost

Note add the following event to the end of the MembershipFactory::joinDao function to monitor the contract state:

//@audit added to track mints
emit MintNumber(daos[daoMembershipAddress].tiers[tierIndex].minted);
//@audit FINDING PoC
// to run the test use the command below
// anvil
// npx hardhat test --grep "Erase MembershipFactory account for tiers minted" --network localhost
describe('Upgrading Tiers', function () {
it('Erase MembershipFactory account for tiers minted', async function () {
//User will join tier 2, which we will later remove
const tierIndex = 2;
//minting user currency token to pay for membership
await testERC20.mint(await addr1.address, ethers.utils.parseEther('400'));
//approving ERC20 token
await testERC20
.connect(addr1)
.approve(membershipFactory.address, TierConfig[tierIndex].price * 2); // Assume approve function exists in CurrencyManager
await currencyManager.addCurrency(testERC20.address); // Assume addCurrency function exists in CurrencyManager
​
//creating a DAO membership with test default config (which should have 3 tiers)
const response = await membershipFactory.createNewDAOMembership(
DAOConfig,
TierConfig
);
//Getting the event data
const receipt = await response.wait();
//this is the address of the proxy from the event data
console.log(
'The address of the proxy ERC1155 token is: ',
receipt.events[7].args[1]
);
//getting the address from the event data
const proxyAddress = receipt.events[7].args[1];
//addr1 has joined the DAO in tier 2
await expect(
membershipFactory.connect(addr1).joinDAO(proxyAddress, tierIndex)
).to.emit(membershipFactory, 'UserJoinedDAO');
​
//Now there will only be one tier. The tier addr1 is in is removed
const newTierConfig = [{ price: 150, amount: 20, minted: 0, power: 4 }];
await expect(
membershipFactory.updateDAOMembership('testdao.eth', newTierConfig)
).to.not.be.reverted;
​
//The tier is added back/modifed again
await expect(
membershipFactory.updateDAOMembership('testdao.eth', TierConfig)
).to.not.be.reverted;
//extracting structs is hard with ethers
/* const currentDaoConfig = await membershipFactory.daos(proxyAddress);
console.log(currentDaoConfig); */
​
const joinResponse = await membershipFactory
.connect(addr1)
.joinDAO(proxyAddress, tierIndex);
​
const joinReceipt = await joinResponse.wait();
const totalAmountMintedForTierTwo = joinReceipt.events[4].args[0]; //according to the MembershipFactory
console.log(
'The total minted amount for Tier 2 is: ',
totalAmountMintedForTierTwo
);
​
//according to the contract the balance of the addr1 is;
​
const proxyV1 = await MembershipERC1155.attach(proxyAddress);
const addr1Balance = await proxyV1.balanceOf(await addr1.address, 2);
​
//you can see that the values are incorrect
console.log('Balance of the addr1 for Tier 2 is: ', addr1Balance);
​
expect(addr1Balance).to.equal(totalAmountMintedForTierTwo);
});
});
​

Impact

The max number of members for a tier can be will be ignored and allow more than desired amount of members in the DAO

Tools Used

hardhat, foundry and manual review

Recommendations

When increasing the amount of tiers for a DAO check the balance for a token in the MembershipERC1155 contract and updated the minted amount for the tier.

Updates

Lead Judging Commences

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

Support

FAQs

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