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

User data can be tampered in case `updateDAOMembership` upgrades DAO to have less tiers than before.

tdey

Summary

If a dao is created with n tiers and it is later being updated to have tiers less than n then user data will be lost

Vulnerability details

The MembershipFactory::updateDAOMembership function, lets EXTERNAL_CALLER upgrade the tier configuration of a DAO.
Let us suppose a DAO is created with name DAO_One having 6 tiers 0 to 5 using the MembershipFactory::createNewDAOMembership function.
And 20 users join spread across different tiers of the DAO.

Now EXTERNAL_CALLER calls updateDAOMembership function and updates the configuration of DAO_One to now have 3 tiers. Anyone with the EXTERNAL_CALLER role can do that and there is no mechanism in the updateDAOMembership function to prevent that from happening.

This can lead to the following issues:

  1. If we see at the updateDAOMembership function it deletes the tiers and completely updates the DAO according to the new configuration.

    delete dao.tiers;

  • If we consider the above mentioned assumptions, and the tiers in the dao is reduced then there is no mechanism present to retrieve the users who were already present in the tiers earlier. If there were 6 tiers earlier and now it's 3 then 3 tiers are being deleted, there is no way to retrieve or track the users who were in the tiers which got deleted.

  • It raises another concern too, even if there was a mechanism to track and re-distribute the users who joined in the tiers which are about to get deleted, it will be against the will of the user. If a user has chosen a particular tier they should remain until they want to change.

  1. It gives unilateral authority to anyone with the EXTERNAL_CALLER role to just do whatever they want with a particular DAO. Even if the role is trusted someone can just do it by mistake at the cost of user data. If we consider that certain functionality is required to update the dao configuration in future it should be solely given to the creator of the DAO.

POC

I have used foundry for writing the test, it is being installed using forge init --force we use the --force flag because it is a non-empty directory. The following test file MembershipFactoryTest includes the setup and the test function

// SPDX-License-Identifier: MIT
pragma solidity 0.8.28;
​
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 {MockUSDC} from "./mocks/MockUSDC.sol";
import {ERC1155Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC1155/ERC1155Upgradeable.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../node_modules/@openzeppelin/contracts/token/ERC1155/IERC1155.sol";
​
contract MembershipFactoryTest is Test {
// actors
address OWPTeam = makeAddr("1WPTeam");
address OWPWallet = makeAddr("OWPWallet");
address DAOCreator = makeAddr("DAOCreator");
address DAOMember = makeAddr("DAOMember");
address usdcDeployer = makeAddr("usdcDeployer");
address ExternalCaller = makeAddr("ExternalCaller");
address OWPFactory = makeAddr("OWPFactory");
​
//contracts
CurrencyManager currencyManager;
MembershipERC1155 membershipERC1155;
MembershipFactory membershipFactory;
MockUSDC usdc;
ERC1155Upgradeable erc1155up;
​
address daoProxy;
function setUp() public {
// Deploy necessary contract
vm.prank(usdcDeployer);
usdc = new MockUSDC();
usdc.mint(DAOMember, 2e18);
​
vm.startPrank(OWPTeam);
currencyManager = new CurrencyManager();
membershipERC1155 = new MembershipERC1155();
bytes32 slot = bytes32(uint256(4));
vm.store(address(membershipERC1155), slot, bytes32(uint256(1000)));
membershipFactory = new MembershipFactory(
address(currencyManager), OWPWallet, "https://images.com/{id}", address(membershipERC1155)
);
//Whitelist usdc
currencyManager.addCurrency(address(usdc));
membershipFactory.grantRole(membershipFactory.EXTERNAL_CALLER(), ExternalCaller);
​
vm.stopPrank();
}
​
function testupdateDAOMembership() public {
​
//make a DAO
TierConfig[] memory tiersInput = new TierConfig[]();
tiersInput[0] = TierConfig({amount: 10, price: 10, power: 2, minted: 0});
tiersInput[1] = TierConfig({amount: 10, price: 5, power: 1, minted: 0});
tiersInput[2] = TierConfig({amount: 10, price: 15, power: 1, minted: 0});
tiersInput[3] = TierConfig({amount: 10, price: 3, power: 1, minted: 0});
tiersInput[4] = TierConfig({amount: 10, price: 5, power: 1, minted: 0});
tiersInput[5] = TierConfig({amount: 10, price: 7, power: 1, minted: 0});
​
DAOInputConfig memory daoInputConfig = DAOInputConfig({
ensname: "firstDao",
daoType: DAOType.PUBLIC,
currency: address(usdc),
maxMembers: 200,
noOfTiers: 6
});
​
vm.prank(DAOCreator);
daoProxy = membershipFactory.createNewDAOMembership(daoInputConfig, tiersInput);
​
//enter 10 customers in spread across different tiers
address[] memory members = new address[]();
members[0] = DAOMember;
members[1] = address(0xB0B);
members[2] = address(0xC0C);
members[3] = address(0xD0D);
members[4] = address(0xE0E);
members[5] = address(0xF0F);
members[6] = address(0x1010);
members[7] = address(0x2020);
members[8] = address(0x3030);
members[9] = address(0x4040);
​
for (uint256 i = 0; i < members.length; i++) {
deal(address(usdc), members[i], 100);
}
​
// Each member joins the DAO at a different tier
for (uint256 i = 1; i == members.length; i++) {
vm.startPrank(members[i]);
IERC20(usdc).approve(address(membershipFactory), 100);
membershipFactory.joinDAO(daoProxy, i % 6); // Join at tier index i % 6
vm.stopPrank();
​
uint256 NFTbalance = MembershipERC1155(daoProxy).balanceOf(members[i], i % 6);
//to check that the members have received Membership1155 tokens
assertEq(NFTbalance, 1);
}
//make new tierConfig
TierConfig[] memory newTiersInput = new TierConfig[]();
newTiersInput[0] = TierConfig({amount: 1, price: 64, power: 64, minted: 0});
newTiersInput[1] = TierConfig({amount: 2, price: 32, power: 32, minted: 0});
newTiersInput[2] = TierConfig({amount: 4, price: 16, power: 16, minted: 0});
​
//get ens name
string memory ensName = daoInputConfig.ensname;
​
//get ExternalCaller role
vm.prank(ExternalCaller);
//call update DAO function and update the dao with new tier configuration
address updatedDAOAddress = membershipFactory.updateDAOMembership(ensName, newTiersInput);
console.log("Address of updated dao:", updatedDAOAddress);
}
}

In the test we first try to make a dao with 6 tiers and join 10 users across different tiers. Then we update the dao with a new tier configuration having 3 tiers.
We run the test by forge test --mt testupdateDAOMembership -vvv we get this as the output :

[PASS] testupdateDAOMembership() (gas: 2980345)
Logs:
Address of updated dao: 0xB2299eb83f16CcEbab40fa2416ECA0aC1d89E24D

We can see that tiers reduced successfully with anyone having the EXTERNAL_CALLER role. There is no mechanism or check to prevent reducing the number of tiers which can hamper the users.

Impact

  1. Users data will be effected and it can lead to unexpected behavior of the protocol

  2. Anyone with EXTERNAL_CALLER role can unilaterally update the DAO configuration whenever they want.

Likelihood: high, Impact: high but this is an access control bug hence I am keeping severity as medium.

Tools used

Manual review

Recommended mitigation

  1. Have a check so that tiers can only be increased and not decreased

  2. The way current updating works can hamper the users, in case of dire need only the creator of the DAO should be able to make the necessary changes.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge
7 months ago
0xbrivan2 Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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