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

Calling `MembershipFactory::updateDAOMembership` To Downgrade DAO Allows Deleted Tier Holders to Retain Privileges

farismaulana

Summary

The updateDAOMembership function in the MembershipFactory contract does not properly handle the downgrading of DAO tiers. Holders of the deleted tiers can still retain all privileges, including claiming profits with shares of the corresponding tier, even though the tier is no longer accessible. This can lead to unfair distribution of profits and 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

there are issue if the MembershipFactory::updateDAOMembership is used to downgrade the DAO tiers, where the holder of tier that would be deleted can still have all the privilege of claiming profit with shares of the corresponding tier. even though the tier is no longer accessible.

add this code to test folder:

ERC20Mock.sol:

// SPDX-License-Identifier: MIT
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:

// SPDX-License-Identifier: MIT
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_downgradingDAOTierUsing_updateDAOMembership() public {
vm.startPrank(alice);
DAOInputConfig memory daoConfig = DAOInputConfig("DAO", DAOType.PUBLIC, address(currency), 100, 3);
TierConfig[] memory tiersConfig = new TierConfig[]();
// amount 10, price 100, power 30, minted 0
tiersConfig[0] = TierConfig(10, 100, 30, 0);
// amount 10, price 100, power 20, minted 0
tiersConfig[1] = TierConfig(10, 100, 20, 0);
// amount 10, price 100, power 10, minted 0
tiersConfig[2] = TierConfig(10, 100, 10, 0);
​
address daoAddress = membershipFactory.createNewDAOMembership(daoConfig, tiersConfig);
vm.stopPrank();
​
vm.startPrank(bob);
// get nft of tier 3
uint256 tierIndex = 2;
currency.approve(address(membershipFactory), 100e18);
membershipFactory.joinDAO(daoAddress, tierIndex);
vm.stopPrank();
​
vm.startPrank(owner);
// downgrade 3 tier to 2 tier
TierConfig[] memory newTiersConfig = new TierConfig[]();
// amount 10, price 100, power 30, minted 0
newTiersConfig[0] = TierConfig(10, 100, 30, 0);
// amount 10, price 100, power 20, minted 0
newTiersConfig[1] = TierConfig(10, 100, 20, 0);
membershipFactory.updateDAOMembership("DAO", newTiersConfig);
vm.stopPrank();
​
vm.startPrank(charlie);
currency.approve(address(membershipFactory), 100e18);
// get nft of tier 2
tierIndex = 1;
membershipFactory.joinDAO(daoAddress, tierIndex);
vm.stopPrank();
​
// incoming profit
vm.startPrank(owner);
currency.approve(daoAddress, 10e18);
MembershipERC1155(daoAddress).sendProfit(10e18);
console2.log("total supply:", MembershipERC1155(daoAddress).totalSupply());
console2.log("total profit:", MembershipERC1155(daoAddress).totalProfit());
console2.log("profit of bob:", MembershipERC1155(daoAddress).profitOf(bob));
console2.log("profit of charlie:", MembershipERC1155(daoAddress).profitOf(charlie));
vm.stopPrank();
​
// bob and charlie claim profit
vm.startPrank(bob);
uint256 bobBalanceBefore = currency.balanceOf(bob);
MembershipERC1155(daoAddress).claimProfit();
uint256 bobBalanceAfter = currency.balanceOf(bob);
​
vm.startPrank(charlie);
uint256 charlieBalanceBefore = currency.balanceOf(charlie);
MembershipERC1155(daoAddress).claimProfit();
uint256 charlieBalanceAfter = currency.balanceOf(charlie);
​
console2.log("Bob balance change: ", bobBalanceAfter - bobBalanceBefore);
console2.log("Charlie balance change: ", charlieBalanceAfter - charlieBalanceBefore);
}
}
​

using forge test --mt test_POC_downgradingDAOTierUsing_updateDAOMembership -vv we can check that bob can still claimProfit.

[PASS] test_POC_downgradingDAOTierUsing_updateDAOMembership() (gas: 1578024)
Logs:
total supply: 48
total profit: 208333333333333333333333333333333333333333333333
profit of bob (tier 3 which is not accesible anymore): 3333333333333333333
profit of charlie (tier 2): 6666666666666666666
Bob balance change: 3333333333333333333
Charlie balance change: 6666666666666666666

Impact

Holders of the deleted tiers can still retain all privileges, including claiming profits with shares of the corresponding tier.

Tools Used

foundry

Recommendations

  1. calling claimProfit there should be check whether the tier is inside the active tiers

Updates

Lead Judging Commences

0xbrivan2 Lead Judge
7 months ago
0xbrivan2 Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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