Feat: Funding Pot #741

fabianschu · 2025-04-30T22:36:49Z

No description provided.

src/modules/logicModule/interfaces/ILM_PC_FundingPot_v1.sol

src/modules/logicModule/LM_PC_FundingPot_v1.sol

0xNuggan · 2025-05-04T09:12:34Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        }
+
+        if (block.timestamp > round_.roundStart) {
+            revert Module__LM_PC_FundingPot__RoundAlreadyStarted();


A bit of a nitpick, but since this will also trigger if somebody tries to edit an already finished round, maybe we can change the name (or add an extra error if size allows for it?)

I can't really come up with a more descriptive error since the idea is that the admin can edit as long as a round hasn't started. Imo the error exactly tells this story.

Yeah, fair. My comment came from the thought of "if a round started and ended, it's weird to revert saying 'it started' ". But it makes sense, the condition is "rounds are editable before start". Let's leavi it like that.

src/modules/logicModule/LM_PC_FundingPot_v1.sol

0xNuggan

In general looks good, there are a couple of things I pointed out. I also have still an open question about the caps/accesstypes, but I wanted to get this out first

0xNuggan · 2025-05-23T17:55:25Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        }
+
+        if (block.timestamp > round_.roundStart) {
+            revert Module__LM_PC_FundingPot__RoundAlreadyStarted();


Yeah, fair. My comment came from the thought of "if a round started and ended, it's weird to revert saying 'it started' ". But it makes sense, the condition is "rounds are editable before start". Let's leavi it like that.

omega-audits · 2025-05-25T18:15:06Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+    // Constants
+
+    /// @notice The role that allows creating funding rounds.
+    bytes32 public constant FUNDING_POT_ADMIN_ROLE = "FUNDING_POT_ADMIN";


You will need to update it for the new authorization logic

omega-audits · 2025-05-25T18:17:26Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+    }
+
+    /// @inheritdoc ILM_PC_FundingPot_v1
+    function getRoundAccessCriteria(uint32 roundId_, uint8 accessCriteriaId_)


Should revert if the roundId_ or its accessCriteriaId_ don't exist

(Unless returning all empty over revert is intended)

I think we were trying to reduce contract size here and so adding the revert may actually push us over the limit, if it doesn't then I think changing it makes sense but if it does then we'll have to leave as is.

so this function si not sued internally, so there is no internal logic problem.
To me, also returning isList_ = true when the AccessCriteriaType is equal to OPEN is unexpected, but it is impossible to say if this is problematic as we do not know the use case. What is the use case?

omega-audits · 2025-05-25T18:28:44Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+    ) external onlyModuleRole(FUNDING_POT_ADMIN_ROLE) {
+        Round storage round = rounds[roundId_];
+
+        if (accessCriteriaType_ > MAX_ACCESS_CRITERIA_ID) {


Here it checks against the type not the id. According to the name and other uses of MAX_ACCESS_CRITERIA_ID, it should both check here against accessCriteriaId_ instead of accessCriteriaType_, and in if accessCriteriaId_ is 0 it should check roundIdToNextAccessCriteriaId won't be higher.

Though it's a bit unclear why require it to be up to 4?
Was that meant to be limiting the type or the ID in fact?

Nice find, we fixed this in the fixes PR

omega-audits · 2025-05-25T18:31:22Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        // Clear all existing data to prevent stale data
+        round.accessCriterias[criteriaId].nftContract = address(0);
+        round.accessCriterias[criteriaId].merkleRoot = bytes32(0);
+        // @note: When changing allowlists, call removeAllowlistedAddresses first to clear previous entries


Shouldn't this be called here then to make sure it's removed?

We need to add a note here to be more explicit but we won't be fixing this for now.

this should not be problematic, as these data are read only if the relevant Type is set.
However, the logic is not consistent, and that can be confusing for someone who calls this function.
you could simplify this block of code (i.e. these reset lines plus the stuff on l 531f) by just these variables to the value of the parameters.

omega-audits · 2025-05-25T20:28:11Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        _validateEditRoundParameters(round);
+
+        for (uint i = 0; i < addressesToRemove_.length; i++) {
+            unchecked {


What's this unchecked for? (there's no math operation inside here)

omega-audits · 2025-05-25T20:38:28Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+                    Module__LM_PC_FundingPot__UnspentCapsRoundIdsNotStrictlyIncreasing(
+                );
+            }
+


Could move lastSeenRoundId = currentProcessingRoundId; here and delete it from the 3 other places it's done below to avoid code repetition.

omega-audits · 2025-05-25T20:41:36Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+    {
+        Round storage round = rounds[roundId_];
+
+        if (round.roundEnd == 0 && round.roundCap == 0) {


Instead of this, you could just check if the round ID is lower than the current one in the counter (here and in other places this check is done)

Or check roundStart != 0 as it cannot be 0 when a round is added

omega-audits · 2025-05-25T20:46:52Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        uint contributorCount = contributors.length;
+
+        // Check batch size is not zero
+        if (batchSize_ == 0 || batchSize_ > contributorCount) {


It might make operations more convenient to set batchSize_ to contributorCount instead of reverting if it's bigger

omega-audits · 2025-05-25T21:21:28Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        }
+
+        // --- Personal Cap Check ---
+        {


Should have an if here to check if personal cap exists/ should be applied

hmmm now sure this is needed here's why

userPersonalCap will be 0 (default value)

If a user tries to contribute any amount (let's say 100 tokens)

The code checks: userPreviousContribution >= userPersonalCap

Since userPersonalCap is 0, and any contribution > 0, this will be true

The transaction will revert with PersonalCapReached

Omega Audit Fixes

jellegerbrandy · 2025-06-06T16:43:11Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        view
+        returns (bool isEligible, uint remainingAmountAllowedToContribute)
+    {
+        if (accessCriteriaId_ > MAX_ACCESS_CRITERIA_TYPE) {


so here you compare accessCriteriaId with MAX_ACCESS_CRITERIA_TYPE, while in setAccessCriteria, it is the accessCriteriaType_ argument (not the id) that is supposed to be smaller than MAX_ACCESS_CRITERIA_TYPE.

Not sure what the original intention is here, but you should decide if the constnat MAX_ACCESS_CRITERIA_TYPE limits the ids or the type (I guess the latter)

removed the if check, since it is not required for accessCriteriaId

jellegerbrandy · 2025-06-06T16:50:21Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

@@ -0,0 +1,1438 @@
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity 0.8.23;


as a general remark, it is better (easier to review, easier to understand, easier to version control) to split the logic over diffferent classes/files rather than mixing all together in one huge file.

jellegerbrandy · 2025-06-06T16:53:07Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+    mapping(
+        uint32 roundId
+            => mapping(uint8 accessCriteriaId_ => AccessCriteriaPrivileges)
+    ) private roundIdToAccessCriteriaIdToPrivileges;


why put this in a separate mapping? is it not cleaner to have the priviliges part of the accessCriteria object instead of createing a separate mapping?

yes for sure good note but we will not be fixing this time around

jellegerbrandy · 2025-06-06T17:12:06Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        Round storage round = rounds[roundId_];
+
+        if (accessCriteriaType_ > MAX_ACCESS_CRITERIA_TYPE) {
+            revert Module__LM_PC_FundingPot__InvalidAccessCriteriaId();


perhaps not the error you want to use here, as the id could be fine..

Changed the error string from Module__LM_PC_FundingPot__InvalidAccessCriteriaId to Module__LM_PC_FundingPot__InvalidAccessCriteriaType

jellegerbrandy · 2025-06-06T17:13:43Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        // Otherwise, edit the existing one
+        if (accessCriteriaId_ == 0) {
+            unchecked {
+                criteriaId = ++roundIdToNextAccessCriteriaId[roundId_];


if you have prblems witht he contract size, a small gain is to have a global counter for teh criteriaId instead of a spearate one for each round.

thanks good to note but a little too late for us to change here, will keep noted for the next contract

jellegerbrandy · 2025-06-06T17:22:27Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+            round.accessCriterias[criteriaId].merkleRoot = merkleRoot_;
+        } else if (accessCriteriaType == AccessCriteriaType.LIST) {
+            // For LIST type, update the allowed addresses
+            for (uint i = 0; i < allowedAddresses_.length; i++) {


it is important to note here that you are NOT overwriting the allowedlist here, but only adding to it. This is not obvious at all. I'd recomend to adda "addressesToRemove" parameter as well to the function signature to make this explicit.

Added the fix and supporting unit test as well!

jellegerbrandy · 2025-06-09T15:18:20Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+        uint unspentPersonalCap = 0; // Initialize to 0
+        uint32 lastSeenRoundId = 0; // Tracks the last seen roundId to ensure strictly increasing order
+
+        // Process each previous round cap that the user wants to carry over


I do not see where it is enforced taht the unspentPersonalRoundCaps are "previous" to the given roundId_. This is important, as in this way, they could include the roundId_ itself (and so the personal cap would be counted twice), or even after roundId_ (if roundId_ has overrideContributionSpan set to true)

I have pushed a fix for this, and added a test as well

jellegerbrandy · 2025-06-09T15:19:24Z

src/modules/logicModule/LM_PC_FundingPot_v1.sol

+                    .accessCriteriaId];
+
+                uint userContribution =
+                    roundIdToUserToContribution[currentProcessingRoundId][user_];


roundIdToUserToContribution does not seem to be updated for the currentPRocessingRoundId. That would mean that the personalCap of previous rounds can be used over and over again.

Do you have a proposed fix in mind as to how to fix this? some pseudo code would be helpful as well @jellegerbrandy

I have pushed a fix for this, and added a couple of tests.

* fix: new audit fixes 1 * fix: new audit fixes 2 * fix: new audit fixes 3 * fix: audit fixes 4 * fix: new audit fixes 4 -remove unneccessary code * fix: new audit fixes 5 * fix: new audit fixes 5: add getter function * fix: contract size fix 1 * fix: contract size fix 2: remove getter functions and make public * fix: contract size fix 3 * fix: contract size fix 3: if condition * contract resizing * chore:remove uncommented tests * chore:remove hook validation --------- Co-authored-by: Zuhaib Mohammed <zzzuhaibmohd@gmail.com> Co-authored-by: Jeffrey Owoloko <72028836+JeffreyJoel@users.noreply.github.com> Co-authored-by: JeffreyJoel <jowoloko@gmail.com>

fabianschu requested a review from 0xNuggan April 30, 2025 22:37

0xNuggan reviewed May 5, 2025

View reviewed changes

0xNuggan requested changes May 5, 2025

View reviewed changes

zzzuhaibmohd and others added 27 commits May 22, 2025 14:46

init: create the funding pot v1 template

a20b087

feat: funding pot admin authorization

fdec788

test: add tests for funding pot contract

5fea6e6

feat: add checks for granting and revoking roles

64fdd6a

feat: add funding pot admin role check

c641ddd

test: add gherkin guide and unit tests

5da9352

fix: add missing internal function to exposed_

a07596a

chore: remove redundant template code

29a6971

test: remove unused test code

307303d

test: code format and unit tests

0afb10c

chore: apply PR review changes

6d2fe12

Test: Add test for internal function

3388d2f

chore: remove redundant code

edadf3a

chore: inverter standard change

2deacdc

Feat: Add round creation and edit functions

03694d1

feat: create and edit rounds implementation

cf292e5

test: add tests for the edit rounds functionality

ce9b04a

fix: follow inverter standard and remove redundant code

a5b4ce7

fix: follow inverter standard and remove redundant code

10cc1ea

fix: follow inverter standard and remove redundant code

b058ca0

feat: add access criteria implementation

23097bf

test: code format & access control criteria unit tests

5962650

fix: inverter standard changes

72a3f18

fix: add bool to getRoundAccessCriteria()

e683568

fix: inverter standard changes

48cedee

fix: inverter standard changes

3179ce5

fix: PR Review Fix#1

9c8e42a

fabianschu added 9 commits May 22, 2025 14:49

fix: max personal cap defined by highest access criteria

4145835

chore: test structure cleanup

a943aef

chore: fmt

198f733

chore: removes redundant code

fb1ad69

fix: compiler warnings

e29de85

chore: remove redundant check on zero contribution

240f52e

chore: fmt

2a5371f

fix: >= instead of ==

6d3384b

fix: merge conflicts

8dda04f

fabianschu force-pushed the feat/funding-pot branch from f8d3200 to 8dda04f Compare May 22, 2025 15:32

fabianschu requested a review from 0xNuggan May 22, 2025 15:35

0xNuggan approved these changes May 23, 2025

View reviewed changes

omega-audits reviewed May 25, 2025

View reviewed changes

zzzuhaibmohd and others added 3 commits May 27, 2025 20:49

fix: audit fixes #1

7940d76

fix: audit fixes 2

87cebb9

Merge pull request #757 from 33Audits/feat/funding-pot

b519b35

Omega Audit Fixes

jellegerbrandy reviewed Jun 6, 2025

View reviewed changes

jellegerbrandy reviewed Jun 9, 2025

View reviewed changes

zzzuhaibmohd mentioned this pull request Jun 18, 2025

New Audit Fixes #762

Closed

leeftk and others added 3 commits September 16, 2025 09:25

chore: format

3305f5a

Merge branch 'dev' into feat/funding-pot

18e1464

0xNuggan merged commit 2c05ca6 into dev Sep 17, 2025
5 checks passed

0xNuggan deleted the feat/funding-pot branch September 17, 2025 07:04

		@@ -0,0 +1,1438 @@
		// SPDX-License-Identifier: LGPL-3.0-only
		pragma solidity 0.8.23;

Feat: Funding Pot #741

Feat: Funding Pot #741

Uh oh!

Conversation

fabianschu commented Apr 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xNuggan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzuhaibmohd Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzuhaibmohd Jun 11, 2025 •

edited

Loading