This Low severity report consist of 4 different issues found within the codebase.

NOTE:The 2 last ones which have a high impact, but still are not likely to happen, due to some constrainst but that definitely enters Low severity. They points out the lack of enforcement of key values leading to a permanent DOS and distrubtion of a functionality and the second one leads to a direct theft of funds.

NOTE2: The DOS issue has similarities with cyfin finding 7.3.6, also this issue gave the ability to increase the impact of another issue (already reported), it should implement the fix to avoid a totally incorrect state due to the violation of the invariant/check within the joinDAO() that ensures that amount cannot be set under the already minted value hence this should be taken further than a simple "admin input" issue.

1. Duplicate event UserJoinedDAO()

Summary

Wrong name assigned event UserJoinedDAO() could cause problems with offchain applications/Dapps, and mislead the end user.

Vulnerability Details

In MembershipERC1155::joinDAO() function, the event is defined as follow:

But this has been wrongly been duplicated to the MembershipERC1155::upgradeTier() function:

Impact

The upgradeTier() is a important function. And it has the same name event as another totally different function. The incorrect event logs may cause off-chain services to malfunction or mislead users.

Tools Used

Manual Review

Recommendations

2. Partial implementation of SafeERC20

Summary

SafeERC20 openzeppelin's library has been correctly implemented in the MembershipERC1155 contract for safer ERC20 transfer operation. But has not been implemented in the MembershipFactory contract while ERC20 operations are being done with the same currencies leading to dangerous situation.

Vulnerability Details

Looking at theMembershipERC1155 contract where SafeERC20 have been implemented, which allows the contract to integrate any ERC20:

While in the MembershipFactory contract the basic ERC20 library is being used leading to inconsistency and future issues when implementing some other ERC20 (USDT, etc):

Impact

• Inconsistency

• If any other "uncommon" ERC20 like USDT is implemented, transfer would only work on one side of the protocol

Tools Used

Manual Review

Recommendations

3. Lack of enforcement for amount in updateDAOMembership enables permanent DOS when admin wants to lower the amount

Summary

When updating DAO tier configurations, the updateDAOMembership function fails to validate that new tier amounts are greater than or equal to the number of already minted tokens. This allows tier amounts to be set below currently minted values, permanently blocking new mints due to the minted counter never decreasing.

Vulnerability Details

Let's take a look at how updateDAOMembership() handles tier updates:

NOTE: any value set for .amount that is under the current minted amount is enough to trigger the DOS

DOS Scenario:

1. Initial state:
   => Tier 1 has amount=50, minted=40

2. Admin updates configuration:
   => Sets new amount=35 for Tier 1
   => minted stays at 40 (previous value)

3. Random user call joinDAO function

1. joinDAO() check fails:
   => require(35 > 40, "Tier full") // Always reverts

Moreover minted never decreases, meaning that the DOS would remain permanently even if Tokens are burned, this is actually a second critical issue that makes the DOS permanent as long no the intervention of another update. So the main impact is that the function would not be available due to the new state.

Impact

Permanent DOS on one of the protocol's main functionality (joinDAO) due to lack of validation coupled of another issue which is minted variable never decrement, even when tokens are being burned.

Tools Used

Manual review

Recommendations

1. The main recommendation would be to validate the amount field to ensure it doesn't break the invariant within the joinDAO function.

4. Implementation allows tierConfigs.price to be empty, could lead to the DAO being drained

Summary

NOTE: this issue doesn't rely on the owner being malicious or passing wrong data, but it englobes a combination favorable factors that could lead to all funds being drained. The main factor is that the function wipes the whole previous configuration and doesn't make sure that a price is being set for each tier. Because the price is the most critical weak point of the whole system, it should at least check if it is initialized. While it requires only 1 tier (even the lowest level) with no price value, for the funds being drained.

When updating DAO tier configurations, the contract updateDAOMembership() dangerously wipe every value from each tier and doesn't validate at all wether tier price are set. This could allow attackers to mint unlimited tokens for free and draining funds within MembershipERC1155.

Vulnerability Details

Let's take a look at how updateDAOMembership() handles the update:

Notice delete dao.tiers, at this point each field is in a unitialized state. This line is very dangerous and a factor increasing the likelihood of the issue. Because it assumes that admin is willing to update every single tier, while he might have the intend to update a couple of tiers (e.g tier 4 and 5).

Then dao.tiers.push(tierConfigs[i]);, pushes the updated tiers to the daoConfig. But fails to ensure that each price field is set to a value for each tierConfigs. An attacker would have the ability to mint an unlimited amount of tokens at no price and drain all of the funds within the MembershipERC1155 contract.

Impact

At this point anyone would be able to steal all the funds from the affacted MembershipERC1155 contract.

Tools Used

Manual review

Recommendations

Ensure that priceis being checked to avoid such likely event to happen, ultimately implement the possibility for the admin to chose which tier he is willing to update instead of wiping the whole prev config.