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

Possible DoS for sponsored DAO's members upgrading their tier via `MembershipFactory::upgradeTier` due to an arithmetic underflow in `MembershipERC1155::burn_`.

v1vah0us3

Summary

A potential Denial of Service (DoS) vulnerability exists in the MembershipFactory::upgradeTier function, triggered by an arithmetic underflow in the MembershipERC1155::burn_ function. This issue occurs when a member of a sponsored DAO attempts to upgrade their membership tier, but the transaction fails due to an underflow in the calculation of MembershipERC1155::totalSupply. This state can be achieved when there are very few users in the entire protocol.

Description

The vulnerability arises from the MembershipERC1155::burn_ function, which contains the following code:

function burn_(address from, uint256 tokenId, uint256 amount) internal {
@> totalSupply -= amount * 2 ** (6 - tokenId);
_burn(from, tokenId, amount);
}

This highlited line of code in MembershipERC1155::burn_ attempts to decrement MembershipERC1155::totalSupply based on the amount of NFTs to burn and the user's tier level. The issue occurs when the function tries to burn NFTs from a user who is a member in the sponsored DAO and wants to upgrade his tier via MembershipFactory::upgradeTier, with very few users in the entire protocol. This action could fail and result in an arithmetic underflow issue.

Specifically, when the user joins a DAO, the corresponding NFT (amount 1) will be minted, as shown in the MembershipFactory::joinDAO function below:

function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
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;
uint256 platformFees = (20 * tierPrice) / 100;
daos[daoMembershipAddress].tiers[tierIndex].minted += 1;
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), owpWallet, platformFees);
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), daoMembershipAddress, tierPrice - platformFees);
@> IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, tierIndex);
}

The MembershipERC1155::mint function updates the MembershipERC1155::totalSupply:

function mint(address to, uint256 tokenId, uint256 amount) external override onlyRole(OWP_FACTORY_ROLE) {
@> totalSupply += amount * 2 ** (6 - tokenId);
_mint(to, tokenId, amount, "");
}

If the user joined the sponsored DAO, he might want to upgrade his tier, via MembershipFactory::upgradeTier function. The function's logic will attempt to burn the user's lower tier NFTs (amount 2) and then mint an NFT for the next tier, thus upgrading the user's tier:

function upgradeTier(address daoMembershipAddress, uint256 fromTierIndex) external {
require(daos[daoMembershipAddress].daoType == DAOType.SPONSORED, "Upgrade not allowed.");
require(daos[daoMembershipAddress].noOfTiers >= fromTierIndex + 1, "No higher tier available.");
@> IMembershipERC1155(daoMembershipAddress).burn(_msgSender(), fromTierIndex, 2);
@> IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), fromTierIndex - 1, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, fromTierIndex - 1);
}

The function's logic depends heavily on the correct execution of MembershipERC1155::burn and the calculation of MembershipERC1155::totalSupply.

So what happens when the entire protocol has a very few users, and one of them who has just joined a newly created sponsored DAO, wants to upgrade his tier by calling the upgradeTier function? This action could result in an underflow and cause the transaction to fail, as the MembershipERC1155::totalSupply value could be less than the calculated amount for the burn, due to this line in the code [https://github.com/Cyfrin/2024-11-one-world/blob/main/contracts/dao/tokens/MembershipERC1155.sol#L74]. This issue could lead to a Denial of Service (DoS) scenario where the upgrade process fails for some users in a new sponsored DAOs attempting to upgrade their tiers.

Attack Scenario

  1. The "OneWorld" protocol has just been launched and the first DAO (which is in your example a sponsored DAO) is created using MembershipFactory::createNewDAOMembership. As there are no DAO members in the whole protocol, the MembershipERC1155::totalSupply will initially be 0.

  2. The first user wishes to join the newly created sponsored DAO using MembershipFactory::joinDAO with a tier index of 1. One NTF will be minted for the user and the MembershipERC1155::totalSupply will have a value of 32, since totalSupply += amount * 2 ** (6 - tokenId).

  3. Shortly afterwards the same user decides to upgrade his tier using MembershipFactory::upgradeTier. So he would have to burn 2 lower tier NFTs, but first the MembershipERC1155::totalSupply would be updated, since totalSupply -= amount * 2 ** (6 - tokenId) and the value of amount is hardcoded to 2. In this particular case, totalSupply would approach a negative value, since 2 * 2 ** (6 - 1) == 64 and 32 - 64 == -32!!! As MembershipERC1155::totalSupply is of type uint256, it cannot hold negative values and the transaction will be rolled back, resulting in a DoS.

  4. In order to upgrade, this member will have to wait until the protocol has enough users to join the DAOs.

PoC

For the test to be successful, the beforeEach section of MembershipFactory.test.ts in DAOConfig should contain noOfTiers: 7, daoType: DAOType.SPONSORED and TierConfig should have exactly 7 configurations, as shown in the example below:

DAOConfig = { ensname: "testdao.eth", daoType: DAOType.SPONSORED, currency: testERC20.address, maxMembers: 100, noOfTiers: 7 };
TierConfig = [{ price: 300, amount: 10, minted: 0, power: 12 }, { price: 200, amount: 10, minted: 0, power:6 }, { price: 100, amount: 10, minted: 0, power: 3 }, { price: 100, amount: 10, minted: 0, power: 3 }, { price: 100, amount: 10, minted: 0, power: 3 }, { price: 100, amount: 10, minted: 0, power: 3 }, { price: 100, amount: 10, minted: 0, power: 3 }];

After the adjustment in MembershipFactory.test.ts is done, the following test can be added to MembershipFactory.test.ts and executed using npx hardhat test test/MembershipFactory.test.ts --network localhost (with running anvil):

describe("Member joins a sponsored DAO and immediately upgrades tier", function () {
it("should allow the first user to join at tier 2, but prevent his upgrade due to an arithmetic underflow", async function () {
// Step 1: Whitelist the test ERC20 currency
console.log("Whitelisting test ERC20 currency...");
await currencyManager.addCurrency(testERC20.address);
const userOne = addr1;
// Step 2: Create the DAO membership and get the DAO contract address
console.log("Creating DAO membership...");
await membershipFactory.connect(userOne).createNewDAOMembership(DAOConfig, TierConfig);
const daoAddress = await membershipFactory.getENSAddress("testdao.eth");
const membershipERC1155 = await MembershipERC1155.attach(daoAddress);
console.log("DAO created at address:", membershipERC1155.address);
// Step 3: Mint tokens for addr1
const mintAmount = ethers.utils.parseUnits("1000", 18);
console.log("Minting tokens for addr1...");
await testERC20.mint(userOne.address, mintAmount);
// Step 4: Approve tokens for the membershipFactory
await testERC20.connect(userOne).approve(membershipFactory.address, mintAmount);
// Verify owner's balance
const ownerBalance = await testERC20.balanceOf(userOne.address);
console.log("Owner's balance after minting:", ethers.utils.formatUnits(ownerBalance, 18), "OWP");
// Step 5: Ensure the allowance is set properly
const allowance = await testERC20.allowance(userOne.address, membershipFactory.address);
console.log("Allowance of addr1 for DAO:", ethers.utils.formatUnits(allowance, 18), "OWP");
expect(allowance).to.equal(mintAmount);
// Step 6: Join the DAO at tier 2 (index 1)
console.log("addr1 joining DAO at tier 2...");
const joinTx = await membershipFactory.connect(userOne).joinDAO(membershipERC1155.address, 1);
console.log("Join transaction sent, waiting for confirmation...");
// Wait for the transaction to be mined and confirmed
const receipt = await joinTx.wait();
console.log("Join transaction mined in block:", receipt.blockNumber);
// Step 7: Attempt to upgrade the tier and expect revert due to the arithmetic underflow
console.log("addr1 attempting to upgrade tier, expecting revert due an arithmetic underflow ");
try {
const upgradeTx = await membershipFactory.connect(userOne).upgradeTier(membershipERC1155.address, 1);
await upgradeTx.wait();
} catch (err) {
// Check if the error is an instance of Error
if (err instanceof Error) {
console.log("Upgrade failed as expected: ", err.message);
} else {
console.log("An unknown error occurred:", err);
}
}
});
});
​

The upgrade will revert as expected due to an arithmetic underflow, here is the log:

Upgrade failed as expected: (reason="execution reverted: panic: arithmetic underflow or overflow (0x11)")

Impact

If an underflow occurs, it will cause the entire transaction to revert, preventing a member from upgrading his tier. This can be problematic in some scenarios where the protocol has a sponsored DAO and yet a very small total number of users. In this case, it may be possible to experience a DoS while attempting to upgrade a tier, as the calculation of totalSupply in MembershipERC1155::burn_ could result in an arithmetic underflow and the entire tier upgrade mechanism could fail (see the "Attack Scenario" and "PoC" sections above). This could temporarily prevent legitimate upgrades in a sponsored DAO.

Tools Used

Manual review, Hardhat, VSCode

Recommendations

Consider updating the MembershipERC1155::burn_ function to include a check for underflows before modifying totalSupply. This can be done by ensuring that the new value of totalSupply does not go negative:

function burn_(address from, uint256 tokenId, uint256 amount) internal {
+ require(totalSupply >= amount * 2 ** (6 - tokenId), "Tier upgrade temporarily unavailable!");
totalSupply -= amount * 2 ** (6 - tokenId);
_burn(from, tokenId, amount);
}
Updates

Lead Judging Commences

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

Appeal created

v1vah0us3 Submitter
10 months ago
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.