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

A race condition between `MembershipFactory::updateDAOMembership` to a lower tier and `MembershipFactory::upgradeTier` causes loss of tier configuration and user privileges

x18a6

Summary

The updateDAOMembership function in MembershipFactory deletes and recreates the tiers array, leading to the loss of data for tiers that are removed when updating from a higher number of tiers to a lower number. This can strand member assets and privileges in the erased tiers.

Vulnerability Details

Cyfrin's report mention under 7.4.4 that DAOs of all types can be updated with a lower number of tiers and are not validated to be above zero, But fails to elaborate on the actual impact of updating to lower tiers. Additionaly, no fix is implemented.

In MembershipFactory, the updateDAOMembership function completely deletes and recreates the tiers array:

contracts/dao/MembershipFactory.sol#L120-L125

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

When updating from n tiers to m tiers where m < n:

  • All n tiers are deleted with delete dao.tiers

  • Only m new tiers are added back

  • Tiers m+1 through n are permanently erased

Next, we demonstrate this is not necesssarily an admin error. Consider the following race condition:

  • A user is upgrading from tier to 1 to 2.

  • An admin is updating the DAO from 2 tiers to 1 tier.

  • Both txs are in the mempool.

If the user tx processes first, it will upgrade to tier 2 only to be erased by the admin tx. With all this in mind, we argue this is a MEDIUM severity issue.

Impact

  • Any members or assets in the deleted tiers lose their tier configuration context, effectively strand member assets and privileges in those erased tiers.

Tools Used

Manual review.

Recommendations

  • Consider allowing tiers to only be upgraded to a higher number of tiers:

function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs) external {
// ...
​
+ require(dao.tiers.length <= tierConfigs.length, "Cannot reduce the number of 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;
}
​
// ...
}

  • Alternatively, ensure that any higher tiers being deleted do not have any existing members before allowing the update.

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.