Summary
The MembershipFactory::updateDAOMembership contract lacks proper validation when updating tier configurations through the given function. Administrators could mistakenly modify tiers in a way that enables users to mint more membership tokens than intended, exceeding the DAO's maxMembers limit. This vulnerability can lead to an over-allocation of membership tokens and undermine the DAO's governance structure.
Vulnerability Details
The problem is presented in MembershipFactory::updateDAOMembership function of the contract. When an administrator updates the tier configurations, there are insufficient checks to ensure that the new amount of a tier configuration is not less than the currently minted tokens and at the same time we don't allow more people to join another tier.
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;
for (uint256 i = 0; i < tierConfigs.length; i++) {
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
}
}
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}
Impact
This vulnerability allows:
Exceeding Membership Limits: Users can mint more tokens than intended, surpassing the DAO's maxMembers limit.
Unfair Advantage: Users may gain excessive influence or benefits within the DAO due to holding more membership tokens.
Undermined Governance: The DAO's governance structure and integrity can be compromised, leading to potential disputes among members.
Proof Of Concept
In the provided proof of concept (PoC), the following steps highlight the vulnerability:
1 - DAO Creation: A DAO is created with a maxMembers limit of 5 and two tiers:
3 - Minting Tokens: A user mints tokens up to the limit in both tiers:
4 - Updating Tiers: The administrator updates the tier configurations:
3 - Additional Minting: The user is now able to mint additional tokens in Tier 1:
5 - Exceeding maxMembers: The user now holds 8 tokens, exceeding the maxMembers limit of 5.
The vulnerability exists because the updateDAOMembership function does not validate whether the new tier amounts, combined with already minted tokens, respect the DAO's maxMembers limit.
Add the following tests to MembershipFactory.test.ts
describe("update_tiers_missing_checks", function () {
it("it fails to create dao if the maximum amount of member for more than 1 tier is set to uint256 max", async function () {
[owner, addr1, addr2, ...addrs] = await ethers.getSigners();
CurrencyManager = await ethers.getContractFactory("CurrencyManager");
currencyManager = await CurrencyManager.deploy();
await currencyManager.deployed();
MembershipERC1155 = await ethers.getContractFactory('MembershipERC1155');
const membershipImplementation = await MembershipERC1155.deploy();
await membershipImplementation.deployed();
MembershipFactory = await ethers.getContractFactory("MembershipFactory");
membershipFactory = await MembershipFactory.deploy(currencyManager.address, owner.address, "https://baseuri.com/", membershipImplementation.address);
await membershipFactory.deployed();
await currencyManager.addCurrency(testERC20.address);
const newDAOConfig = { ensname: "testdao.eth", daoType: DAOType.GENERAL, currency: testERC20.address, maxMembers: 5, noOfTiers: 2 };
const newTierConfig = [{ price: 50, amount: 4, minted: 0, power: 12 }, { price: 25, amount: 1, minted: 0, power: 6 }];
const tx = await membershipFactory.connect(addr1).createNewDAOMembership(newDAOConfig, newTierConfig);
const receipt = await tx.wait();
const event = receipt.events.find((event: any) => event.event === "MembershipDAONFTCreated");
const ensName = event.args[2][0];
const nftAddress = event.args[1];
const ensToAddress = await membershipFactory.getENSAddress("testdao.eth")
expect(ensName).to.equal("testdao.eth");
expect(ensToAddress).to.equal(nftAddress);
await testERC20.mint(owner.address, ethers.utils.parseEther('10000'));
for (let i = 0; i < 4; i++) {
await testERC20.transfer(addr1.address, ethers.utils.parseEther('100'));
await testERC20.connect(addr1).approve(membershipFactory.address, ethers.utils.parseEther('100'));
await membershipFactory.connect(addr1).joinDAO(nftAddress, 0);
}
await testERC20.transfer(addr1.address, ethers.utils.parseEther('100'));
await testERC20.connect(addr1).approve(membershipFactory.address, ethers.utils.parseEther('100'));
await membershipFactory.connect(addr1).joinDAO(nftAddress, 1);
const updateTierConfig = [{ price: 20, amount: 1, minted: 0, power: 4 }, { price: 10, amount: 4, minted: 0, power: 4 }];
await membershipFactory.updateDAOMembership("testdao.eth", updateTierConfig);
for (let i = 0; i < 3; i++) {
await testERC20.transfer(addr1.address, ethers.utils.parseEther('100'));
await testERC20.connect(addr1).approve(membershipFactory.address, ethers.utils.parseEther('100'));
await membershipFactory.connect(addr1).joinDAO(nftAddress, 1);
}
const membershipContract = await ethers.getContractAt("MembershipERC1155", nftAddress);
const balanceOfFirstTier = await membershipContract.balanceOf(addr1.address, 0);
const balanceOfSecondTier = await membershipContract.balanceOf(addr1.address, 1);
expect(balanceOfFirstTier).to.equal(4);
expect(balanceOfSecondTier).to.equal(4);
const daoConfig = await membershipFactory.daos(nftAddress);
expect(balanceOfFirstTier.add(balanceOfSecondTier)).to.be.gt(daoConfig.maxMembers);
});
});
Tools Used
Manual Code Review: Analyzing the smart contract code to identify the lack of validation checks.
Testing Frameworks: Using Hardhat and Chai to replicate the issue and confirm the vulnerability through the provided PoC.
Recommendations
To mitigate this vulnerability:
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) {
+ if (dao.tiers[i].minted > tierConfigs[i].amount) {
+ revert("Cannot reduce tier amount.");
+ }
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]);
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;
}
Potentially, it is a good idea to consider burning minted tokens for users in a tier that has been removed on update.
