Summary
Protocol claims to preserve tiers.minted amount during MembershipFactory::updateDAOMembership, but does follow
through with it, if the tierConfig ordering is different.
Vulnerability Details
Protocol claims to preserve tiers.minted amount during MembershipFactory::updateDAOMembership, but does follow
through with it, if the tierConfig ordering is different.
The updateDAOMembership allows EXTERNAL_CALLER to update DAOMembership by influencing the following,
dao.maxMembers
dao.noOfTiers
dao.tiers.amount
dao.tiers.price
dao.tiers.power
Entities the EXTERNAL_CALLER cannot influence
dao.ensname
dao.daoType
dao.currency
dao.tiers.minted => this is preserved unless EXTERNAL_CALLER downgrades noOfTiers.
The minted amount isn't actually preserved if tierConfig ordering is different. For example,
Let's say alice creates DAO by providing following tierConfigs,
[tier4, tier5, tier6]
Then, userA mints tier6.
tier6.minted becomes 1.
Now, protocol decides to update pricings of the tier for the DAO created by alice, they provide the following configs,
[tier6, tier5, tier4] => with updated prices.
Since the order is reserved (The code logic permits it), minted information is lost.
Instead of getting preserved, tier6.minted becomes 0. This means one additional person can now join the DAO.
Impact
Protocol loses track of actual minted amounts of tokens, resulting in inaccurate records, allowing more mints thanintended, affecting overall governance and management.
Proof-Of-Code
it("tierConfig.minted not preserved during DAO update", async () => {
[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);
DAOConfig1 = { ensname: "testdao.eth", daoType: DAOType.GENERAL, currency: testERC20.address, maxMembers: 70, noOfTiers: 3 };
TierConfig1 = [
{ price: 10, amount: 10, minted: 0, power:1 },
{ price: 20, amount: 20, minted: 0, power:2 },
{ price: 40, amount: 40, minted: 0, power:4 },
];
const tx = await membershipFactory.connect(addr2).createNewDAOMembership(DAOConfig1, TierConfig1);
await tx.wait();
const ensAddress = await membershipFactory.getENSAddress("testdao.eth");
membershipERC1155 = await MembershipERC1155.attach(ensAddress);
await testERC20.mint(addr1.address, ethers.utils.parseEther("300"));
await testERC20.connect(addr1).approve(membershipFactory.address, ethers.utils.parseEther("200"));
await membershipFactory.connect(addr1).joinDAO(membershipERC1155.address, 0);
assert.equal(await membershipFactory.mintedSoFar(membershipERC1155.address, 0), BigInt(1));
const newTierConfig = [
{ price: 400, amount: 40, minted: 0, power:4 },
{ price: 200, amount: 20, minted: 0, power:2 },
{ price: 100, amount: 10, minted: 0, power:1 },
];
await membershipFactory.updateDAOMembership("testdao.eth", newTierConfig);
assert.equal(await membershipFactory.mintedSoFar(membershipERC1155.address, 2), BigInt(0));
});
Tools Used
Manual Review & Hardhat Testing
Recommendations
Consider adding tierLevel in tierConfig that aligns with tierConfigs being provided.
struct TierConfig {
+ uint256 tierLevel; // The tier level (0~6)
uint256 amount; // The total number of tokens available for that tier
uint256 price; // The price of a token in the given currency.
uint256 power; // The voting power of the token.
uint256 minted; // The total number of tokens already minted for that tier
}
function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
...
...
for (uint256 i = 0; i < tierConfigs.length; i++) {
if (i < dao.tiers.length) {
+ uint256 currentTierLevel = dao.tiers[i].tierLevel;
+ uint256 providedTierLevel = tierConfigs[i].tierLevel;
+ require(currentTierLevel == providedTierLevel, "Tier indexing discrepancies detected");
tierConfigs[i].minted = dao.tiers[i].minted;
}
}
}
