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

Lack of TierConfig[].price validation checking while creating or updating DAO can cause huge fund loss for a member.

krishnambstu

Summary

TierConfig[].price is not compared among Tiers when MembershipFactory:createNewDAOMembership() and MembershipFactory:updateDAOMembership() is called thus making financial loss for DAO members and rendering the DAO broken.

Root Cause

MembershipFactory:createNewDAOMembership()L55-L63

MembershipFactory:updateDAOMembership()L100-L134

Vulnerability Details

When a DAO is created or updated there is no validation check on TierConfig[].price. The price of lower indexed tier must be higher than higher indexed tier price which is not enforced in the code. And This lack of validation or misconfigured tiers can cause huge financial loss for a member in several ways.

1) Any member can buy lower indexed Tier membership token with less price and can get higher profit. Any member who tries to buy higher indexed Tier membership token will pay more and get less profit.

2) Any member who would try to upgrade their tier will burn two tokens which he bought with more price and get a token which is of less price. In this case profit share is not changed but the member would loss his fund.

POC

Suppose A DAO creator creates a DAO with Tierconfig[0].price = 100 and Tierconfig[1].price = 200,which is not expected . it is possible to create the DAO, though this is very unlikely if we consider the DAO creator is sensible enough. But we can not rely on this assumption that a human won't make a mistake. And if this is validated Off-Chain in the backend, This still can happen if some one create a DAO directly on chain bypassing Off-Chain as On-chain is independent from Off-chain. While creating DAO this mistake can happen accidentally which will cause huge damage, actually would make the DAO broken. There's a off chain restriction on updateDAOMembership() function which is not the case for createNewDAOMembership() function

function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
//code
}

Make a .t.sol file in test folder and paste the code below then run command: forge test

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;
​
import "forge-std/Test.sol";
import "../contracts/dao/CurrencyManager.sol";
import "../contracts/dao/MembershipFactory.sol";
import "../contracts/dao/libraries/MembershipDAOStructs.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import { OWPERC20 } from "../contracts/shared/testERC20.sol"; // Update paths as needed
​
contract MembershipFactoryTest is Test {
CurrencyManager public currencyManager;
MembershipERC1155 public membershipImplementation;
MembershipFactory public membershipFactory;
address public daoMembershipAddress;
OWPERC20 public testERC20;
address public owner = makeAddr("owner");
address public DAOCreator = makeAddr("DAOCreator");
address public member1 = makeAddr("member1");
DAOConfig public daoconfig;
DAOInputConfig public daoConfig;
TierConfig[] public tierConfig;
​
//Setup function for testing
​
function setUp() public {
vm.startBroadcast(owner);
​
// Deploy CurrencyManager
currencyManager = new CurrencyManager();
​
// Deploy MembershipERC1155
membershipImplementation = new MembershipERC1155();
testERC20 = new OWPERC20("OWP", "OWP");
currencyManager.addCurrency(address(testERC20));
testERC20.mint(member1, 10000000000000000000);
​
// Deploy MembershipFactory
membershipFactory = new MembershipFactory(
address(currencyManager),
address(owner),
"https://baseuri.com/",
address(membershipImplementation)
);
vm.stopBroadcast();
}
​
function testTierConfig() public {
vm.startBroadcast(owner);
​
// Set DAO and Tier Input configurations
daoConfig = DAOInputConfig({
ensname: "testdao.eth",
daoType: DAOType.SPONSORED,
currency: address(testERC20),
maxMembers: 100,
noOfTiers: 7
});
​
tierConfig.push(
TierConfig({ price: 300, amount: 3, minted: 0, power: 12 })
);
//Thse prices are not in the way they should be
//Misconfigure Tiers
tierConfig.push(TierConfig({ price: 100, amount: 3, minted: 0, power: 6 }));
tierConfig.push(TierConfig({ price: 200, amount: 3, minted: 0, power: 3 }));
tierConfig.push(TierConfig({ price: 300, amount: 3, minted: 0, power: 3 }));
tierConfig.push(TierConfig({ price: 400, amount: 3, minted: 0, power: 3 }));
tierConfig.push(TierConfig({ price: 500, amount: 3, minted: 0, power: 3 }));
tierConfig.push(TierConfig({ price: 600, amount: 2, minted: 0, power: 3 }));
​
//creating new DAO
daoMembershipAddress = membershipFactory.createNewDAOMembership(
daoConfig,
tierConfig
);
vm.stopBroadcast();
​
//Member1 joining DAO
vm.startBroadcast(member1);
testERC20.approve(address(membershipFactory), 1000000000);
​
membershipFactory.joinDAO(daoMembershipAddress, 4);
membershipFactory.joinDAO(daoMembershipAddress, 5);
membershipFactory.joinDAO(daoMembershipAddress, 6);
membershipFactory.joinDAO(daoMembershipAddress, 6);
​
//upgrading Tier
membershipFactory.upgradeTier(daoMembershipAddress, 6);
​
vm.stopBroadcast();
}
}
​

Impact

Though Likelihood is low but the impact is critical causing financial loss for the members, breaking the invariant of the protocol which is allowing a DAO to be totally broken.

Tools Used

Manual review, foundry

Recommendations

While creating or updating DAO every consequent tier price must be checked and validated.

Add this code both in createNewDAOMembership() function and updateDAOMembership() function

for (uint256 i = 0; i < tierConfigs.length - 1; i++) {
require(tierConfigs[i].price > tierConfigs[i + 1].price);
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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

Give us feedback!