Summary
The updateDAOMembership function allows the EXTERNAL_CALLER role to update a DAO's tierConfig. This function can delete the old dao.tiers and updates these values with the inputted tierConfigs if tierConfigs.length is less than dao.tiers.length. This will lead to users being members in a tier that no longer exists, keep claiming profits and vote on DAO decisions.
Vulnerability Details
The updateDAOMembership function allows the EXTERNAL_CALLER role to update a DAO's tierConfig.
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;
}
As observed in this function, this function does not check for the existing length of the dao.tiers.
In an example scenario where this DAO's DAOType is not SPONSORED and the existing tier length is 7 when the inputted tierConfigs length is 6, function will loop through the tier configs in tierConfigs, update the minted status for all items in this array and delete dao.tiers.
As observed, due to the new amount of tiers being less than the existing amount of tiers, lowest tier will not be accounted for and be deleted.
Users that are in Tier 7 are not accounted for, they will keep their ERC1155 tokens, keep accumulating profits and will still be able to vote in DAO decisions even when this tier no longer exists and no new users can join this tier. Taking a look at the functions in MembershipERC1155.sol contract we can observe that this is true.
function claimProfit() external returns (uint256 profit) {
profit = saveProfit(msg.sender);
require(profit > 0, "No profit available");
savedProfit[msg.sender] = 0;
IERC20(currency).safeTransfer(msg.sender, profit);
emit Claim(msg.sender, profit);
}
function profitOf(address account) external view returns (uint256) {
return savedProfit[account] + getUnsaved(account);
}
function getUnsaved(address account) internal view returns (uint256 profit) {
return ((totalProfit - lastProfit[account]) * shareOf(account)) / ACCURACY;
}
function shareOf(address account) public view returns (uint256) {
return (balanceOf(account, 0) * 64) +
(balanceOf(account, 1) * 32) +
(balanceOf(account, 2) * 16) +
(balanceOf(account, 3) * 8) +
(balanceOf(account, 4) * 4) +
(balanceOf(account, 5) * 2) +
balanceOf(account, 6);
}
Even though there are members part of this non existent tier, no new members will be able to join this tier as observed from the following check in joinDAO function.
function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
require(daos[daoMembershipAddress].noOfTiers > tierIndex, "Invalid tier.");
Proof of Concept
Following test proves it is possible to delete a tier that has members. Keep in mind that ERC1155 tokens minted before the change do not get burnt.
1) Implement a new getter function in MembershipFactory.sol to make tests easier.
function getDAODataFromAddressFromENS(string memory ensName, uint256 tier) public view returns (uint256, uint256) {
address daoAddress = getENSAddress[ensName];
DAOConfig storage dao = daos[daoAddress];
return (dao.tiers[tier].minted, dao.tiers[tier].amount);
}
2) Implement a mock WETH contract for testing inside the test folder.
pragma solidity 0.8.22;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockWETH is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
3) Implement the following foundry test and run it to observe the vulnerability
pragma solidity 0.8.22;
import { Test, console } from "forge-std/Test.sol";
import { MembershipFactory } from "../contracts/dao/MembershipFactory.sol";
import { CurrencyManager } from "../contracts/dao/CurrencyManager.sol";
import { MembershipERC1155 } from "../contracts/dao/tokens/MembershipERC1155.sol";
import { OWPIdentity } from "../contracts/OWPIdentity.sol";
import "../contracts/dao/libraries/MembershipDAOStructs.sol";
import { MockWETH } from "./MockWETH.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract TestOneWorld is Test {
address OWPTeam = makeAddr("1WPTeam");
address OWPWallet = makeAddr("OWPWallet");
address DAOCreator = makeAddr("DAOCreator");
address DAOMember = makeAddr("DAOMember");
address DAOMember2 = makeAddr("DAOMember2");
address DAOMember3 = makeAddr("DAOMember3");
address WETHDeployer = makeAddr("WETHDeployer");
CurrencyManager currencyManager;
MembershipERC1155 membershipERC1155;
MembershipFactory membershipFactory;
MockWETH weth;
address daoSponsoredProxy;
address daoPublicProxy;
function setUp() public {
vm.prank(WETHDeployer);
weth = new MockWETH("Wrapped ETHER", "WETH");
vm.startPrank(OWPTeam);
currencyManager = new CurrencyManager();
membershipERC1155 = new MembershipERC1155();
membershipFactory = new MembershipFactory(
address(currencyManager),
OWPWallet,
"https://images.com/{id}",
address(membershipERC1155)
);
currencyManager.addCurrency(address(weth));
vm.stopPrank();
weth.mint(DAOMember, 1000e18);
weth.mint(DAOMember2, 1000e18);
weth.mint(DAOMember3, 1000e18);
}
function testUpdateDAOToDeleteTierWithMember() public {
TierConfig[] memory tiersInput = new TierConfig[]();
tiersInput[0] = TierConfig({ amount: 1, price: 64, power: 64, minted: 0 });
tiersInput[1] = TierConfig({ amount: 2, price: 32, power: 32, minted: 0 });
tiersInput[2] = TierConfig({ amount: 4, price: 16, power: 16, minted: 0 });
tiersInput[3] = TierConfig({ amount: 8, price: 8, power: 8, minted: 0 });
tiersInput[4] = TierConfig({ amount: 16, price: 4, power: 4, minted: 0 });
tiersInput[5] = TierConfig({ amount: 32, price: 2, power: 2, minted: 0 });
tiersInput[6] = TierConfig({ amount: 64, price: 1, power: 1, minted: 0 });
TierConfig[] memory tiersInput2 = new TierConfig[]();
tiersInput2[0] = TierConfig({ amount: 1, price: 64, power: 64, minted: 0 });
tiersInput2[1] = TierConfig({ amount: 2, price: 32, power: 32, minted: 0 });
tiersInput2[2] = TierConfig({ amount: 4, price: 16, power: 16, minted: 0 });
tiersInput2[3] = TierConfig({ amount: 8, price: 8, power: 8, minted: 0 });
tiersInput2[4] = TierConfig({ amount: 16, price: 4, power: 4, minted: 0 });
tiersInput2[5] = TierConfig({ amount: 32, price: 2, power: 2, minted: 0 });
DAOInputConfig memory daoPublicInputConfig = DAOInputConfig({
ensname: "daoPublic",
daoType: DAOType.PUBLIC,
currency: address(weth),
maxMembers: 300,
noOfTiers: 7
});
vm.prank(DAOCreator);
daoPublicProxy = membershipFactory.createNewDAOMembership(
daoPublicInputConfig,
tiersInput
);
vm.startPrank(DAOMember);
IERC20(weth).approve(address(membershipFactory), 1000e18);
membershipFactory.joinDAO(daoPublicProxy, 6);
(uint256 mintedAmount, ) = membershipFactory.getDAODataFromAddressFromENS("daoPublic", 6);
assertEq(mintedAmount, 1);
vm.stopPrank();
vm.prank(OWPTeam);
membershipFactory.updateDAOMembership("daoPublic", tiersInput2);
vm.prank(DAOMember);
vm.expectRevert("Invalid tier.");
membershipFactory.joinDAO(daoPublicProxy, 6);
}
Impact
User's that have joined lower tiers risk having their tier deleted, no new members will be able to join this tier and there will be members of a tier that doesn't exist. Members of this non existing tier will keep their ability to collect profits and vote in DAO decisions.
Impact: Medium
Likelihood: Medium
Tools Used
Manual review
Recommendations
Make sure that length of tierConfigs matches the length of dao.tiers. An example of this is shown below.
function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
address daoAddress = getENSAddress[ensName];
++ require(tierConfigs.length == dao.tiers.length, "Invalid tier count.");
Alternatively, make sure that when the length of tierConfigs is less than the length of dao.tiers, burn tokens of the members of the lower tiers that are being deleted and refund their funds back to them.
