Summary
In the updateDAOMembership function, there's a critical data inconsistency issue when updating tier configurations. The function allows updating tier amounts without properly validating against existing minted tokens.
Vulnerability Details
tierConfigs[i].minted = dao.tiers[i].minted;
This can lead to a situation where:
Original tier: {amount: 100, minted: 50}
New tier: {amount: 40, minted: 50}
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
Can result in:
Tier amount < already minted tokens
Total supply inconsistency
Broken invariant: minted <= amount
Impact
-
Data Integrity:
Tier amounts can become less than minted tokens
Breaks fundamental token supply invariants
Compromises DAO membership tracking
-
Business Logic:
Incorrect membership limits
Broken access control based on tier ownership
Potential economic impact on DAO governance
-
System State:
Inconsistent membership data
Unreliable tier availability
Compromised maxMembers calculation
Proof of Concept
Add this test to the "Update DAO Membership" describe block
it("Should not allow tier update with amount less than minted tokens", async function () {
const initialTierConfig = [{
price: ethers.utils.parseEther("1"),
amount: 100,
minted: 0,
power: 1
}];
DAOConfig.noOfTiers = 1;
DAOConfig.ensname = "inconsistent.eth";
await currencyManager.addCurrency(testERC20.address);
await membershipFactory.createNewDAOMembership(DAOConfig, initialTierConfig);
const daoAddress = await membershipFactory.getENSAddress("inconsistent.eth");
membershipERC1155 = await MembershipERC1155.attach(daoAddress);
await testERC20.mint(addr1.address, ethers.utils.parseEther("100"));
await testERC20.connect(addr1).approve(
membershipFactory.address,
ethers.utils.parseEther("100")
);
for(let i = 0; i < 50; i++) {
await membershipFactory.connect(addr1).joinDAO(daoAddress, 0);
}
const newTierConfig = [{
price: ethers.utils.parseEther("1"),
amount: 40,
minted: 0,
power: 1
}];
await expect(
membershipFactory.updateDAOMembership("inconsistent.eth", newTierConfig)
).to.be.revertedWith("New amount cannot be less than minted tokens");
});
Tools Used
Manual Review
Recommendations
To mitigate this issue modify the updateDAOMembership function to validate the existing minted value against the new amount.
function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
address daoAddress = getENSAddress[ensName];
require(tierConfigs.length <= TIER_MAX, "Invalid tier count.");
require(tierConfigs.length > 0, "Invalid tier count.");
require(daoAddress != address(0), "DAO does not exist.");
DAOConfig storage dao = daos[daoAddress];
if(dao.daoType == DAOType.SPONSORED){
require(tierConfigs.length == TIER_MAX, "Invalid tier count.");
}
uint256 maxMembers = 0;
- // 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;
- }
- }
+ // Validate existing minted tokens against new amounts and preserve minted value
+ for (uint256 i = 0; i < tierConfigs.length; i++) {
+ uint256 mintedAmount = i < dao.tiers.length ? dao.tiers[i].minted : 0;
+ require(
+ tierConfigs[i].amount >= mintedAmount,
+ "New tier amount must be >= minted tokens"
+ );
+ tierConfigs[i].minted = mintedAmount;
+ }
// 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;
}
// updating the ceiling limit acc to new data
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}
