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

`updateDAOMembership` can cause loss of funds and unintentional tier removal

holydevoti0n

Summary

The updateDAOMembership function of the MembershipFactory contract allows updating a DAO's tier configurations. However, when the new configuration has fewer tiers than the existing ones, the function deletes tiers without adequately preserving data for tiers beyond the new length.

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/MembershipFactory.sol#L114-L125

function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
...
// 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;
}

Notice there is no check to guarantee that tierConfigsand daosare the same size.

When updating to fewer tiers, the function deletes all existing tiers:

// 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;
}

Let's look at the PoC below which shows 2 different problems:

  • 1st: the tokens existing in one tier are lost(protocol cannot track them). Besides, a sendProfit is made and this tier which is inaccessible, receives the profits.

  • 2nd: tier is lost after the update.

  1. Install foundry: foundryup then forge init --force

  2. In the test folder create the file MembershipFactory.t.sol and paste the code below:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import "forge-std/Test.sol";
import "../contracts/dao/MembershipFactory.sol";
import "../contracts/dao/CurrencyManager.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
​
// Mock ERC20 for testing
contract MockERC20 is ERC20 {
constructor() ERC20("Mock Token", "MTK") {
_mint(msg.sender, 1000000 * 10**18);
}
}
​
contract MembershipFactoryTest is Test, ERC1155Holder {
MembershipFactory public factory;
CurrencyManager public currencyManager;
MembershipERC1155 public membershipImpl;
MockERC20 public mockToken;
address public admin = address(this);
address public owpWallet = address(0x123);
​
event MembershipDAONFTCreated(string indexed ensName, address nftAddress, DAOConfig daoData);
​
function setUp() public {
// Deploy mock token
mockToken = new MockERC20();
// Deploy CurrencyManager and whitelist mock token
currencyManager = new CurrencyManager();
currencyManager.addCurrency(address(mockToken));
// Deploy MembershipERC1155 implementation
membershipImpl = new MembershipERC1155();
// Deploy MembershipFactory
factory = new MembershipFactory(
address(currencyManager),
owpWallet,
"https://api.oneworldproject.io/metadata/",
address(membershipImpl)
);
}
​
function test_LostTokensAfterTierUpdate_andProfitIsLost() public {
// Setup initial DAO with 7 tiers
TierConfig[] memory tiers = new TierConfig[]();
for(uint i = 0; i < 7; i++) {
tiers[i] = TierConfig({
amount: 100,
price: 1 ether / (2 ** i),
power: 64 / (2 ** i),
minted: 0
});
}
​
DAOInputConfig memory daoConfig = DAOInputConfig({
ensname: "test.dao",
daoType: DAOType.PUBLIC,
currency: address(mockToken),
maxMembers: 700,
noOfTiers: 7
});
​
address daoAddress = factory.createNewDAOMembership(daoConfig, tiers);
MembershipERC1155 membershipToken = MembershipERC1155(daoAddress);
​
// Setup user
address user = makeAddr("user");
deal(address(mockToken), user, 100 ether);
​
// User joins tier 6 (lowest tier)
vm.startPrank(user);
mockToken.approve(address(factory), 1 ether);
factory.joinDAO(daoAddress, 6);
vm.stopPrank();
​
// Verify initial state
console.log("User balance of tier 6 token:", membershipToken.balanceOf(user, 6));
console.log("Initial tier 6 power:", membershipToken.shareOf(user));
​
// Update to only 3 tiers
TierConfig[] memory newTiers = new TierConfig[]();
for(uint i = 0; i < 3; i++) {
newTiers[i] = TierConfig({
amount: 100,
price: 1 ether / (2 ** i),
power: 64 / (2 ** i),
minted: 0
});
}
​
// Grant role and update
bytes32 EXTERNAL_CALLER = keccak256("EXTERNAL_CALLER");
factory.grantRole(EXTERNAL_CALLER, address(this));
factory.updateDAOMembership("test.dao", newTiers);
​
// Check if user still has power despite tier being deleted
console.log("User balance of tier 6 token after update:", membershipToken.balanceOf(user, 6));
console.log("User power after tier removed:", membershipToken.shareOf(user));
​
// Try to distribute profits to demonstrate impact
address profitSender = makeAddr("profitSender");
deal(address(mockToken), profitSender, 10 ether);
vm.startPrank(profitSender);
mockToken.approve(daoAddress, 10 ether);
membershipToken.sendProfit(10 ether);
vm.stopPrank();
​
console.log("Profit share for non-existent tier: %e", membershipToken.profitOf(user));
}
​
function test_TierIsLostAfterUpdate() public {
// Setup initial DAO with 7 tiers
TierConfig[] memory tiers = new TierConfig[]();
for(uint i = 0; i < 7; i++) {
tiers[i] = TierConfig({
amount: 100,
price: 1 ether / (2 ** i),
power: 64 / (2 ** i),
minted: 0
});
}
​
DAOInputConfig memory daoConfig = DAOInputConfig({
ensname: "test.dao",
daoType: DAOType.PUBLIC, // Using PUBLIC to allow tier updates
currency: address(mockToken),
maxMembers: 700,
noOfTiers: 7
});
​
// Create DAO
address daoAddress = factory.createNewDAOMembership(daoConfig, tiers);
​
// Setup users
address user = makeAddr("user");
deal(address(mockToken), user, 100 ether);
​
// User joins highest and lowest tiers
vm.startPrank(user);
mockToken.approve(address(factory), 2 ether);
factory.joinDAO(daoAddress, 0); // Join tier 0
factory.joinDAO(daoAddress, 6); // Join tier 6
vm.stopPrank();
​
// Get current tiers and verify minted amounts
TierConfig[] memory currentTiers = factory.tiers(daoAddress);
assertEq(currentTiers[0].minted, 1, "Tier 0 should have 1 token minted");
assertEq(currentTiers[6].minted, 1, "Tier 6 should have 1 token minted");
​
// Try to update with fewer tiers
TierConfig[] memory newTiers = new TierConfig[]();
for(uint i = 0; i < 3; i++) {
newTiers[i] = TierConfig({
amount: 100,
price: 1 ether / (2 ** i),
power: 64 / (2 ** i),
minted: 0
});
}
​
// Grant EXTERNAL_CALLER role to test account to call updateDAOMembership
bytes32 EXTERNAL_CALLER = keccak256("EXTERNAL_CALLER");
factory.grantRole(EXTERNAL_CALLER, address(this));
// Update DAO membership
factory.updateDAOMembership("test.dao", newTiers);
​
// Verify new tier configuration
TierConfig[] memory updatedTiers = factory.tiers(daoAddress);
// Tier lost its minted tokens
console.log("Number of tiers after update:", updatedTiers.length);
console.log("Tier 0 minted tokens:", updatedTiers[0].minted);
// Try to join to the valid tier
vm.startPrank(user);
mockToken.approve(address(factory), 1 ether);
vm.expectRevert("Invalid tier.");
factory.joinDAO(daoAddress, 6);
vm.stopPrank();
}
​
}

Then run: forge test --match-test test_ -vv

Output:

Ran 2 tests for test/MembershipFactory.t.sol:MembershipFactoryTest
[PASS] test_LostTokensAfterTierUpdate_andProfitIsLost() (gas: 1961172)
Logs:
User balance of tier 6 token: 1
Initial tier 6 power: 1
User balance of tier 6 token after update: 1
@> User power after tier removed: 1 // tier is lost, no one can join, but user has funds
@> Profit share for non-existent tier: 1e19 // lost tier received profit
​
[PASS] test_TierIsLostAfterUpdate() (gas: 1824628)
Logs:
@> Number of tiers after update: 3 // we started with 7 tiers, but 4 tiers are gone
Tier 0 minted tokens: 1
​
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 2.97ms (2.93ms CPU time)

Impact

  • Members retain their ERC1155 tokens but lose the associated tier data in dao.tiers.

  • Members cannot transfer, burn, or use their tokens.

  • Profit calculations will be broken as they will distribute shares for deleted/unaccessible tiers.

  • DoS - users cannot upgrade/join to the lost tier.

The likelihood is low, but the impact is high. Thus, Medium severity.

Tools Used

Manual Review & Foundry

Recommendations

Before iterating the tierConfig and deleting current tiers, add the following check:

+ // Prevent removing tiers with minted tokens
+ require(tierConfigs.length >= dao.tiers.length, "Cannot remove tiers with minted tokens");
Updates

Lead Judging Commences

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.