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

MembershipFactory::updateDAOMembership issues

decap

Vulnerability Details

There are multiple issues while updating DAO membership

1.It is possible to provide less tiers than it was before. It can lead to scenario when user ends up with token from tiers that don't exist

PoC

Use following test file

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import { Test, console } from "lib/forge-std/src/Test.sol";
import {MockUSDC} from "./mocks/MockUSDC.sol";
​
import {MembershipFactory} from "../contracts/dao/MembershipFactory.sol";
import {CurrencyManager} from "../contracts/dao/CurrencyManager.sol";
import {MembershipERC1155} from "../contracts/dao/tokens/MembershipERC1155.sol";
​
import {DAOInputConfig, DAOType, DAOConfig, TierConfig} from "../contracts/dao/libraries/MembershipDAOStructs.sol";
​
contract Membership is Test{
​
address factoryOwner = makeAddr("factoryOwner");
address daoCreator = makeAddr("daoCreator");
address owpWallet = makeAddr("owpWallet");
​
address daoUser1 = makeAddr("daoUser1");
address daoUser2 = makeAddr("daoUser2");
​
MockUSDC usdc;
CurrencyManager currencyManager;
MembershipFactory membershipFactory;
MembershipERC1155 membershipToken;
​
DAOInputConfig daoConfig;
TierConfig[] tiers;
string daoName = "DaoName";
​
function setUp() public {
membershipToken = new MembershipERC1155();
vm.startPrank(factoryOwner);
usdc = new MockUSDC();
currencyManager = new CurrencyManager();
currencyManager.addCurrency(address(usdc));
membershipFactory = new MembershipFactory(address(currencyManager), owpWallet, "", address(membershipToken));
vm.stopPrank();
​
usdc.mint(daoUser1, 100e6);
​
daoConfig = DAOInputConfig({
ensname: daoName,
daoType: DAOType.PUBLIC,
currency: address(usdc),
maxMembers: 100,
noOfTiers: 4
});
​
TierConfig memory tier1 = TierConfig({
amount: 3,
price: 3e6,
power: 3,
minted: 0
});
tiers.push(tier1);
​
TierConfig memory tier2 = TierConfig({
amount: 3,
price: 2e6,
power: 2,
minted: 0
});
tiers.push(tier2);
​
TierConfig memory tier3 = TierConfig({
amount: 4,
price: 1e6,
power: 1,
minted: 0
});
tiers.push(tier3);
​
TierConfig memory tier4 = TierConfig({
amount: 10,
price: 1e6,
power: 1,
minted: 0
});
tiers.push(tier4);
}
​
function test_shrinkTiers() public {
vm.startPrank(daoCreator);
address daoAddress = membershipFactory.createNewDAOMembership(daoConfig, tiers);
vm.stopPrank();
​
//minting some tier4 tokens for user1
vm.startPrank(daoUser1);
usdc.approve(address(membershipFactory), type(uint256).max);
membershipFactory.joinDAO(daoAddress, 3);
membershipFactory.joinDAO(daoAddress, 3);
vm.stopPrank();
​
//removing some tiers
tiers.pop();
tiers.pop();
​
vm.startPrank(factoryOwner);
membershipFactory.updateDAOMembership(daoName, tiers);
vm.stopPrank();
​
//current length is 2
console.log("Number of tiers after update: %s", membershipFactory.tiers(daoAddress).length);
​
// user holds tokens of tier4
// this line will result with out-of-bounds error
//console.log(membershipFactory.tiers(daoAddress)[3].minted);
}
}

With mock

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import { console } from "lib/forge-std/src/console.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockUSDC is ERC20 {
constructor() ERC20("USDC", "USDC") {
_mint(msg.sender, 1_000_000e6);
}
​
function mint(address to, uint256 amount) public {
_mint(to, amount);
}
}

Run command forge test --mt test_shrinkTiers -vvv

Result:

Logs:
Number of tiers after update: 2

2.It is possible to set non-zero minted value for newly added tier. It can block minting new tokens for newly added tier.

PoC

Add following test to script above:

function test_newTiersCanHaveNonZeroMintedValue() public {
vm.startPrank(daoCreator);
address daoAddress = membershipFactory.createNewDAOMembership(daoConfig, tiers);
vm.stopPrank();
​
//creating new tier than can have non-zero minted value
TierConfig memory tier5 = TierConfig({
amount: 10,
price: 1e6,
power: 1,
minted: 5
});
tiers.push(tier5);
​
//updating dao membership with corrupted tier
vm.startPrank(factoryOwner);
membershipFactory.updateDAOMembership(daoName, tiers);
vm.stopPrank();
​
console.log("Number of tiers after update: %s", membershipFactory.tiers(daoAddress).length);
console.log("Tier5 amount: %s", membershipFactory.tiers(daoAddress)[4].amount);
console.log("Tier5 minted: %s", membershipFactory.tiers(daoAddress)[4].minted);
}

run forge test --mt test_newTiersCanHaveNonZeroMintedValue -vvv

Result:

Logs:
Number of tiers after update: 5
Tier5 amount: 10
Tier5 minted: 5

3.It is possible to update existing tier with amountvalue lower than minted

PoC

Add following test:

function test_higherMintedThanAmount() public {
vm.startPrank(daoCreator);
address daoAddress = membershipFactory.createNewDAOMembership(daoConfig, tiers);
vm.stopPrank();
​
//minting some tier4 tokens for user1
vm.startPrank(daoUser1);
usdc.approve(address(membershipFactory), type(uint256).max);
membershipFactory.joinDAO(daoAddress, 3);
membershipFactory.joinDAO(daoAddress, 3);
membershipFactory.joinDAO(daoAddress, 3);
membershipFactory.joinDAO(daoAddress, 3);
vm.stopPrank();
​
//removing tier4
tiers.pop();
​
//adding updated tier4 to array where amount is lower than current minted
TierConfig memory tier4 = TierConfig({
amount: 2,
price: 1e6,
power: 1,
minted: 0
});
tiers.push(tier4);
​
vm.startPrank(factoryOwner);
membershipFactory.updateDAOMembership(daoName, tiers);
vm.stopPrank();
​
console.log("Tier4 amount: %s", membershipFactory.tiers(daoAddress)[3].amount);
console.log("Tier4 minted: %s", membershipFactory.tiers(daoAddress)[3].minted);
}

run forge test --mt test_higherMintedThanAmount -vvv

Result:

Logs:
Tier4 amount: 2
Tier4 minted: 4

Impact

Medium

Tools Used

Manual review

Recommendations

  1. When updatingDao allow only scenario when number of new provided tiers >= old tiers

  2. If number of new tiers is higher than old ones, check if minted value in new tiers is 0

  3. Add condition that checks if provided amountin tier is higher than minted

Updates

Lead Judging Commences

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

Appeal created

0xbrivan2 Lead Judge
10 months ago
0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

minted value is not asserted to be zero when adding new tiers

Support

FAQs

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