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

`MembershipFactory::joinDAO()` - Any `tierPrice < 5` will result in `platformFees == 0` due to rounding error and lack of using the appropriate math library to handle this properly.

0xscsamurai

https://github.com/Cyfrin/2024-11-one-world/blob/02b59f43981d247caee9aa9ab68d286ce7844a77/contracts/dao/MembershipFactory.sol#L144

Unless the protocol plans to never have tier prices less than 5 (of whatever tokens), this is a bug and should be fixed.

  • Impact: medium

  • Likelihood: medium

  • Severity: medium

The line of code with this rounding down to zero issue:

uint256 platformFees = (20 * tierPrice) / 100;

Proof of Concept (PoC Tests):

Added this to the function in question for my tests:

uint256 platformFees = (20 * tierPrice) / 100;
+ require(platformFees != 0, "Invalid tier price, platformFees == 0!");

and this to the typescript test to listen for the require() revert:

//await expect(membershipFactory.connect(addr1).joinDAO(membershipERC1155.address, tierIndex)).to.emit(membershipFactory, "UserJoinedDAO");
+ await expect(membershipFactory.connect(addr1).joinDAO(membershipERC1155.address, tierIndex)).to.be.revertedWith("Invalid tier price, platformFees == 0!");

Test 1 (control test): using tierPrice == 5, this should not revert.

Using:
https://github.com/Cyfrin/2024-11-one-world/blob/02b59f43981d247caee9aa9ab68d286ce7844a77/test/MembershipFactory.test.ts#L30

TierConfig = [{ price: 5, amount: 10, minted: 0, power: 12 }, { price: 200, amount: 10, minted: 0, power:6 }, { price: 100, amount: 10, minted: 0, power: 3 }];

Test result:

$ yarn test --grep "Should allow user to join DAO" --network localhost
yarn run v1.22.22
warning package.json: No license field
$ hardhat test --grep 'Should allow user to join DAO' --network localhost
​
​
MembershipFactory
Join DAO
1) Should allow user to join DAO
​
​
0 passing (12s)
1 failing
​
1) MembershipFactory
Join DAO
Should allow user to join DAO:
AssertionError: Expected transaction to be reverted with reason 'Invalid tier price, platformFees == 0!', but it didn't revert
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Context.<anonymous> (test/MembershipFactory.test.ts:162:7)
​
​
​
error Command failed with exit code 1.

  • As expected because no rounding down to zero happened, so platformFees > 0. My require check did not revert.

Test 2: using tierPrice < 5, this should revert due to my added require() statement which my test is listening for:

Using:
https://github.com/Cyfrin/2024-11-one-world/blob/02b59f43981d247caee9aa9ab68d286ce7844a77/test/MembershipFactory.test.ts#L30

TierConfig = [{ price: 4, amount: 10, minted: 0, power: 12 }, { price: 200, amount: 10, minted: 0, power:6 }, { price: 100, amount: 10, minted: 0, power: 3 }];

Test result:

$ yarn test --grep "Should allow user to join DAO" --network localhost
yarn run v1.22.22
warning package.json: No license field
$ hardhat test --grep 'Should allow user to join DAO' --network localhost
​
​
MembershipFactory
Join DAO
✔ Should allow user to join DAO
​
​
1 passing (12s)
​
Done in 12.72s.

  • My require check reverted, because platformFees == 0 due to the rounding error for tierPrice == 4

Suggested fix:

Probably make use of the relevant math library suited to handling this type of situation, unless you never plan to have tier prices less than 5...

Updates

Lead Judging Commences

0xbrivan2 Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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