Summary
The MembershipFactory::updateDAOMembership does not validate the minted parameter in the tierConfigs struct for new tiers. This allows arbitrary values to be set for the minted parameter, leading to discrepancies between the recorded minted value and the actual number of tokens minted. This can undermine the integrity of the DAO's membership system, potentially causing issues with governance, distribution of rewards, and overall trust in the protocol.
Vulnerability Details
MembershipFactory.sol#L100-L134
function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external
onlyRole(EXTERNAL_CALLER)
returns (address)
{
address daoAddress = getENSAddress[ensName];
require(tierConfigs.length <= TIER_MAX, "Invalid tier count.");
require(tierConfigs.length > 0, "Invalid tier count.");
require(daoAddress != address(0), "DAO does not exist.");
DAOConfig storage dao = daos[daoAddress];
if (dao.daoType == DAOType.SPONSORED) {
require(tierConfigs.length == TIER_MAX, "Invalid tier count.");
}
uint256 maxMembers = 0;
@> for (uint256 i = 0; i < tierConfigs.length; i++) {
@> if (i < dao.tiers.length) {
@> tierConfigs[i].minted = dao.tiers[i].minted;
}
}
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
if (maxMembers > dao.maxMembers) {
dao.maxMembers = maxMembers;
}
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}
When using updateDAOMembership to update the tier configuration, there are potential flaw where tierConfigs minted amount can be set with any arbitrary number if the config is in new tierConfigs.
The issue is within the marked line, whereas the tierConfigs.length index is checked againts dao.tiers.length or the current state variables.
But the function fails to check whether the new tier config on tierConfigs minted amount is equal to zero or not.
This would lead to the minted number is not the same as the actual number, causing discrepancy.
example scenario:
alice create DAO with 3 tier
owner updates the DAO to 4 tier
using minted value for tier 4 to 50 instead of 0
add this code to test folder:
ERC20Mock.sol:
pragma solidity ^0.8.0;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract ERC20Mock is ERC20 {
constructor(string memory name, string memory symbol, uint256 initialSupply) ERC20(name, symbol) {
_mint(msg.sender, initialSupply);
}
function mint(address to, uint256 amount) public {
_mint(to, amount);
}
function burn(address from, uint256 amount) public {
_burn(from, amount);
}
}
Base.t.sol:
pragma solidity 0.8.22;
import {MembershipFactory} from "../contracts/dao/MembershipFactory.sol";
import {CurrencyManager} from "../contracts/dao/CurrencyManager.sol";
import {DAOType, DAOConfig, DAOInputConfig, TierConfig} from "../contracts/dao/libraries/MembershipDAOStructs.sol";
import {MembershipERC1155} from "../contracts/dao/tokens/MembershipERC1155.sol";
import {Test} from "lib/forge-std/src/Test.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {console2} from "lib/forge-std/src/console2.sol";
contract Base is Test {
string baseURI = "0x";
address owpWallet = makeAddr("owpWallet");
address owner = makeAddr("owner");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
MembershipERC1155 membershiImplementation;
MembershipFactory membershipFactory;
CurrencyManager currencyManager;
ERC20Mock currency;
event UserJoinedDAO(address indexed user, address indexed membershipNftAddress, uint256 tierIndex);
function setUp() public {
vm.startPrank(owner);
membershiImplementation = new MembershipERC1155();
currencyManager = new CurrencyManager();
currency = new ERC20Mock("Currency", "CUR", 100e18);
membershipFactory =
new MembershipFactory(address(currencyManager), owpWallet, baseURI, address(membershiImplementation));
currencyManager.addCurrency(address(currency));
currency.mint(alice, 100e18);
currency.mint(bob, 100e18);
currency.mint(charlie, 100e18);
vm.stopPrank();
}
function test_POC_updateDAOMembership_noCheckOnTierConfigParams() public {
vm.startPrank(alice);
DAOInputConfig memory daoConfig = DAOInputConfig("DAO", DAOType.PUBLIC, address(currency), 100, 3);
TierConfig[] memory tiersConfig = new TierConfig[]();
tiersConfig[0] = TierConfig(10, 100, 30, 0);
tiersConfig[1] = TierConfig(10, 100, 20, 0);
tiersConfig[2] = TierConfig(10, 100, 10, 0);
address daoAddress = membershipFactory.createNewDAOMembership(daoConfig, tiersConfig);
vm.stopPrank();
vm.startPrank(owner);
TierConfig[] memory newTiersConfig = new TierConfig[]();
newTiersConfig[0] = TierConfig(10, 100, 30, 0);
newTiersConfig[1] = TierConfig(10, 100, 20, 0);
newTiersConfig[2] = TierConfig(10, 100, 10, 0);
newTiersConfig[3] = TierConfig(10, 100, 5, 50);
membershipFactory.updateDAOMembership("DAO", newTiersConfig);
vm.stopPrank();
TierConfig[] memory tiers = membershipFactory.tiers(daoAddress);
assertEq(tiers[3].minted, 50);
}
}
run the command forge test --mt test_POC_updateDAOMembership_noCheckOnTierConfigParams and the test would pass indicating the minted amount for tier 4 equal to 50.
Impact
This would cause discrepancies between the recorded minted value and the actual number of tokens minted. Such discrepancies can undermine the integrity of the DAO's membership system, potentially leading to issues with governance, distribution of rewards, and overall trust in the protocol.
Tools Used
foundry
Recommendations
we can enforce the minted amount to zero for new tiers:
function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
address daoAddress = getENSAddress[ensName];
require(tierConfigs.length <= TIER_MAX, "Invalid tier count.");
require(tierConfigs.length > 0, "Invalid tier count.");
require(daoAddress != address(0), "DAO does not exist.");
DAOConfig storage dao = daos[daoAddress];
if(dao.daoType == DAOType.SPONSORED){
require(tierConfigs.length == TIER_MAX, "Invalid tier count.");
}
@@ -113,19 +139,23 @@ contract MembershipFactory is AccessControl, NativeMetaTransaction {
// Preserve minted values and adjust the length of dao.tiers
for (uint256 i = 0; i < tierConfigs.length; i++) {
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
+ } else {
+ tierConfigs[i].minted = 0;
}
}
