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

Misleading tierConfig index allocation leads to users minting different tier than intended

falsegenius

Summary

Incorrect tierIndex representation of a tier user wants to mint could potentially result in users paying for a higher tier & and minting a lower (better) tier.

Vulnerability Details

The MembershipFactory::createNewDAOMembership allows users to create DAOMemberships by providing the tier configs
that align with the DAOType. Different tiers have different prices & benefits that come along. The tier0 is considered the
highest tier. Tier6 is the lowest tier. While creating a DAOMembership, as long as DAOTYPE isn't sponsored, Membership creator can provide optional scalable tiers (any from 0 to 6) that's displayed on frontend as 1 to 7,

PUBLIC -
​
Visible
Scalable (1-7 tiers optional)
Mcap to raise (variable)
​
PRIVATE -
​
Non Visible to non members
Scalable (Tiers 1-7)
Mcap to raise (variable)

This creates a problem in MembershipFactory::joinDAO where user is expected to provide tierIndex to join the Membership.

For example:


Let's say user creates DAO, providing tierConfigs of length 3, with configurations of tiers 4, 5, 6 at index 0, 1, 2 respectively since, in MembershipFactory::createNewDAOMembership, nothing is stopping them from doing this. So this is what's stored in dao.tiers when membership is created,


idx0 => tier4
idx1 => tier5
idx2 => tier6


as evident by the current if statement code block,

for (uint256 i = 0; i < tierConfigs.length; i++) {
require(tierConfigs[i].minted == 0, "Invalid tier config");
dao.tiers.push(tierConfigs[i]);
}
​

Now, if user wants to join, let's say tier5, they would provide idx1 as tierIndex because that's where tier5 is in tiers
array,

@> daos[daoMembershipAddress].tiers[tierIndex]

The current layout for the example is this for daos[daoMembershipAddress].tiers,

=> tiers = [Tier4Config, Tier5Config, Tier6Config]
^
user wants this ------------------

and joinDAO uses daos[daoMembershipAddress].tiers[tierIndex] to get information such as price, amount, minted etc., related to the tier so if user wants tier5, they'd provide tierIndex == 1, not the tierIndex == 5,

require(daos[daoMembershipAddress].noOfTiers > tierIndex, "Invalid tier.");
require(daos[daoMembershipAddress].tiers[tierIndex].amount > daos[daoMembershipAddress].tiers[tierIndex].minted, "Tier full.");
uint256 tierPrice = daos[daoMembershipAddress].tiers[tierIndex].price;

That's where the issue lies because the tierIndex provided is what gets minted in joinDAO,

@> IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);

This means, due to indexing discrepancies, if idx1 is provided as tierIndex, user pays for tier5, but mints tier1!

Currently, there are no measures in MembershipFactory::createNewDAOMembership to ensure that tier0 config is at index 0, tier1 config is at index 1, and so on, in the tierConfigs provided by DAO creator and providing configs is optional for PUBLIC & PRIVATE DAO.

Users who understand this discrepancy can exploit by intentionally selecting indexes that give them higher tier, while paying for a lower price (More highlighted on this in POC & Recommendations section).

Impact

Users could mint higher tiers at incorrect prices. The market could become flooded with users in possession of high-tiers memberships at lower prices. This devalues the memberships, leading to users minting unintended tiers & earning unfair profits.

Tools Used

Manual Review & Hardhat Testing

Proof-Of-Code

  1. DAO creator creates DAO with tierConfigs = [Tier4Config, Tier5Config, Tier6Config].

  2. User interested in Tier5Config provides tierIndex == 1 in joinDAO.

  3. Protocol mints tier1 for the user while they paid for tier5.

The test is conducted against localhost (anvil).

Add the following test in MembershipFactory.test.ts in describe("Create New DAO Membership", function () {},

​
it("Misleading tierConfig index allocation leads to users minting different tier than intended", 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);
​
// 1. Creator creates Membership that includes tiers 4, 5, 6
DAOConfig1 = { ensname: "testdao.eth", daoType: DAOType.GENERAL, currency: testERC20.address, maxMembers: 70, noOfTiers: 3 };
TierConfig1 = [ { price: ethers.utils.parseEther("400"), amount: 40, minted: 0, power:4 },
{ price: ethers.utils.parseEther("200"), amount: 20, minted: 0, power:2 },
{ price: ethers.utils.parseEther("100"), amount: 10, minted: 0, power: 1 }
];
​
const tx = await membershipFactory.connect(addr2).createNewDAOMembership(DAOConfig1, TierConfig1);
await tx.wait();
​
const ensAddress = await membershipFactory.getENSAddress("testdao.eth");
membershipERC1155 = await MembershipERC1155.attach(ensAddress);
​
assert.equal(await membershipERC1155.totalSupply(), 0);
await testERC20.mint(addr1.address, ethers.utils.parseEther("300"));
await testERC20.connect(addr1).approve(membershipFactory.address, ethers.utils.parseEther("200"));
// 2. User is interested in tier5 so they provide tierIndex == 1 because that's where tier5 config is stored for the
// membership in question.
await membershipFactory.connect(addr1).joinDAO(membershipERC1155.address, 1);
​
// 3. TotalSupply is 32 which means tier1 is minted while user paid for tier5
assert.equal(await membershipERC1155.totalSupply(), 32);
assert.equal(await membershipERC1155.balanceOf(addr1.address, 1), 1);
})
​

Recommendations

Consider including tierLevel in TierConfig which could be used to get the tierLevel when tierIndex is provided in joinDAO.

​
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 joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
...
...
+ uint256 tierLevel = daos[daoMembershipAddress].tiers[tierIndex].tierLevel;
- IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
+ IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierLevel, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, tierIndex);
​
}
​
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

falsegenius Submitter
about 1 year ago
falsegenius Submitter
about 1 year ago
0xbrivan2 Lead Judge
about 1 year ago
0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!