Submission Details
Severity:
The function `MembershipFactory::updateDAOMembership()` lacks `TierConfig.minted` checks and relevant constraints, leading to potential update errors and unintended data changes.
Summary
The function MembershipFactory::updateDAOMembership() lacks TierConfig.minted checks and relevant constraints, leading to potential update errors and unintended data changes.
Vulnerability Details
The following function source code highlights potential issues with tier configuration updates. As indicated by @> markers, the tierConfigs.length may not match dao.tiers.length. Although minted values in dao.tiers[i] are synchronized when they overlap, cases where tierConfigs.length is greater than dao.tiers.length do not perform minted-related checks, potentially causing inaccurate updates. Furthermore, if tierConfigs.length is less than dao.tiers.length, the existing minted data may be inadvertently overwritten.
// MembershipFactory::updateDAOMembership()
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;
// 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;
}
}
// Reset and update the tiers array
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
@> dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
// updating the ceiling limit acc to new data
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}
Poc
The following PoC demonstrates two scenarios:
tierConfigs.length > dao.tiers.lengthunintentionally sets minted.tierConfigs.length < dao.tiers.lengthunintentionally clears the existing minted values.
// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.22;
import {Test} from "forge-std/Test.sol";
import {OWPERC20} from "contracts/shared/testERC20.sol";
import {MembershipFactory} from "contracts/dao/MembershipFactory.sol";
import {CurrencyManager} from "contracts/dao/CurrencyManager.sol";
import {MembershipERC1155} from "contracts/dao/tokens/MembershipERC1155.sol";
import { DAOConfig, DAOInputConfig, TierConfig, DAOType, TIER_MAX } from "contracts/dao/libraries/MembershipDAOStructs.sol";
contract MembershipFactoryPoc is Test {
MembershipFactory membershipFactory;
CurrencyManager currencyManager;
MembershipERC1155 membershipImplementation;
address owpWallet = makeAddr("owpWallet");
address membershipNftAddress;
OWPERC20 owpERC20;
address userOne = makeAddr("userOne");
function setUp() public {
// init contracts
owpERC20 = new OWPERC20("OWP","OWP");
currencyManager = new CurrencyManager();
currencyManager.addCurrency(address(owpERC20));
membershipImplementation = new MembershipERC1155();
membershipFactory = new MembershipFactory(address(currencyManager),owpWallet,"",address(membershipImplementation));
// init userOne balance
owpERC20.mint(userOne,100e18);
vm.prank(userOne);
owpERC20.approve(address(membershipFactory),type(uint256).max);
}
function test_updateDAOMembership() public {
// createNewDAOMembership()
DAOInputConfig memory dAOInputConfig = DAOInputConfig("test",DAOType.PUBLIC,address(owpERC20),400,4);
TierConfig[] memory tierConfig = new TierConfig[]();
tierConfig[0] = TierConfig(1,10000,1,0);
tierConfig[1] = TierConfig(1,5000,1,0);
tierConfig[2] = TierConfig(1,2500,1,0);
tierConfig[3] = TierConfig(1,1250,1,0);
membershipNftAddress = membershipFactory.createNewDAOMembership(dAOInputConfig,tierConfig);
// saves the state
uint256 snapshot = vm.snapshotState();
/////////////////////////////////////////////
// tierConfigs.length > dao.tiers.length //
/////////////////////////////////////////////
// updateDAOMembership()
tierConfig = new TierConfig[](5);
tierConfig[0] = TierConfig(1,10000,1,1);
tierConfig[1] = TierConfig(1,5000,1,1);
tierConfig[2] = TierConfig(1,2500,1,1);
tierConfig[3] = TierConfig(1,1250,1,1);
tierConfig[4] = TierConfig(1,625,1,1);
membershipFactory.updateDAOMembership("test",tierConfig);
// assert minted
TierConfig[] memory checkTierConfig = membershipFactory.tiers(membershipNftAddress);
// minted is updated to 1
assertEq(checkTierConfig[4].minted,1);
// useOne can't joinDAO with 4
vm.expectRevert();
vm.prank(userOne);
membershipFactory.joinDAO(address(membershipNftAddress),4);
/////////////////////////////////////////////
// tierConfigs.length < dao.tiers.length //
/////////////////////////////////////////////
// restores the state
vm.revertToState(snapshot);
// useOne joinDAO with 3
vm.prank(userOne);
membershipFactory.joinDAO(address(membershipNftAddress),3);
// userOne has 1 tokens 3
assertEq(MembershipERC1155(membershipNftAddress).balanceOf(userOne,3),1);
// call `updateDAOMembership()` to lower the tier to 3
tierConfig = new TierConfig[](3);
tierConfig[0] = TierConfig(1,10000,1,1);
tierConfig[1] = TierConfig(1,5000,1,1);
tierConfig[2] = TierConfig(1,2500,1,1);
membershipFactory.updateDAOMembership("test",tierConfig);
// call `updateDAOMembership()` to increase the tier to 4
tierConfig = new TierConfig[](4);
tierConfig[0] = TierConfig(1,10000,1,0);
tierConfig[1] = TierConfig(1,5000,1,0);
tierConfig[2] = TierConfig(1,2500,1,0);
tierConfig[3] = TierConfig(1,1250,1,0);
membershipFactory.updateDAOMembership("test",tierConfig);
// userOne gets token 3 again
vm.prank(userOne);
membershipFactory.joinDAO(address(membershipNftAddress),3);
// userOne has 2 tokens 3
assertEq(MembershipERC1155(membershipNftAddress).balanceOf(userOne,3),2);
}
}
// Ran 1 test for test/Poc/MembershipFactoryPoc.t.sol:MembershipFactoryPoc
// [PASS] test_updateDAOMembership() (gas: 1886428)
// Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.76ms (1.12ms CPU time)
Code Snippet
Impact
The lack of minted checks and constraints in MembershipFactory::updateDAOMembership() could lead to accidental overwriting or inappropriate initialization of minted values in TierConfig.
Tools Used
Manual Review
Recommendations
Since TierConfig.amount is modifiable, if a new tier is added, minted should default to 0, and tierConfigs.length should not be less than dao.tiers.length. The following code modifications are recommended:
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];
+ // Prevent the original data from being cleared
+ require(tierConfigs.length >= dao.tiers.length, "Invalid tier count.");
if(dao.daoType == DAOType.SPONSORED){
require(tierConfigs.length == TIER_MAX, "Invalid tier count.");
}
uint256 maxMembers = 0;
// Preserve minted values and adjust the length of dao.tiers
for (uint256 i = 0; i < tierConfigs.length; i++) {
+ // If it is newly added, then its minted should default to 0. If it is not newly added, it will synchronize the original data in if
+ tierConfigs[i].minted = 0;
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
}
}
// Reset and update the tiers array
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
// updating the ceiling limit acc to new data
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}
Updates
Appeal created
joicygiore
10 months ago
0xbrivan2
10 months ago
0xbrivan2
10 months ago
0xbrivan2 10 months ago
Submission Judgement Published Assigned finding tags:
minted value is not asserted to be zero when adding new tiers
Prize pool breakdown
Total prize
15,000 USDC
nSLOC
54128 USDC / LOC
14,000 USDC
1,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.