lemonmon - Missing access modifier for `RFPSimpleStrategy.setPoolActive()` may lead to multiple issues

[sherlock-admin]

lemonmon

high

Missing access modifier for RFPSimpleStrategy.setPoolActive() may lead to multiple issues

RFPSimpleStrategy.setPoolActive() can be called by anybody since it's missing the onlyPoolManager(msg.sender) modifier, which can be abused by a malicious actor to steal funds.

Vulnerability Detail

The comment on line 217 in RFPSimpleStrategy.sol says that 'msg.sender' must be a pool manager in order to be able to call RFPSimpleStrategy.setPoolActive(). However, the necessary onlyPoolManager(msg.sender) modifier is missing.

Impact

Multiple functions inside RFPSimpleStrategy.sol are either using the onlyActivePool or the onlyInactivePool modifiers:

• RFPSimpleStrategy._distribute()
• RFPSimpleStrategy.withdraw()
• RFPSimpleStrategy._registerRecipient()
• RFPSimpleStrategy._allocate()

A malicious actor (Alice) might do the following for example:

1. Alice registers themself as recipient for a RFPSimpleStrategy, specifying a proposalBid which is 15e18.
2. Alice is being declared as the accepted recipient by the pool manager.
3. Now if the tokens were distributed to Alice, the amount of tokens Alice would receive would be (15e18 * milestone.amountPercentage) / 1e18 (line 435 RFPSimpleStrategy.sol).
4. However, Alice calls RFPSimpleStrategy.setPoolActive() to make the pool active again, before the tokens are distributed. Alice might do this by either frontrunning or by executing the tx earlier.
5. Now Alice can call RFPSimpleStrategy._registerRecipient(), since the pool is active again, and Alice re-registers themself but with a higher proposalBid than was accepted before (line 378 RFPSimpleStrategy.sol), for example they re-register with a proposalBid of 60e18.
6. Then Alice calls RFPSimpleStrategy.setPoolActive() to set the pool inactive, so that the tokens can be distributed.
7. Now when the tokens are distributed to Alice for the first milestone (and later also for subsequent milestones), they receive a much higher amount of tokens, since Alice maliciously increased their accepted proposalBid from 15e18 to 60e18, so they would now receive (60e18 * milestone.amountPercentage) / 1e18 (line 435 RFPSimpleStrategy.sol) which is more than was accepted.

The above example illustrates how Alice can abuse setting the pool to active and inactive to change their accepted proposalBid to receive more tokens.

Also, Alice could potentially steal funds from the strategy, if they get accepted with a smaller proposalBid and then maliciously increase the proposalBid as described in the above example, so that Alice would receive a much higher amount of tokens that they are not eligible to receive and that are effectively being stolen from the funds of the strategy.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L217-L221

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L417-L450

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L314-L380

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L386-L393

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L295

Tool used

Manual Review

Recommendation

Consider adding the missing access modifier onlyPoolManager(msg.sender) to RFPSimpleStrategy.setPoolActive().

[jkoppel]

Escalate.

Nearly all the issues marked as duplicates of this issue are not duplicates. The vast majority identify that setPoolActive is missing access control but fail to identify any substantive consequences of this fact. In accordance with Sherlock rules, they should not be marked duplicates.

This was extensively discussed in the Discord. See, for instance, https://discord.com/channels/812037309376495636/1150807984893591643/1154681894261227551 .

[sherlock-admin2]

Escalate.

Nearly all the issues marked as duplicates of this issue are not duplicates. The vast majority identify that setPoolActive is missing access control but fail to identify any substantive consequences of this fact. In accordance with Sherlock rules, they should not be marked duplicates.

This was extensively discussed in the Discord. See, for instance, https://discord.com/channels/812037309376495636/1150807984893591643/1154681894261227551 .

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[jkoppel]

For instance:

I just reviewed the first 20 issues marked as duplicates of this one.

Specifically, I reviewed: #3 , #25 , #29 , #47 , #50 , #55 , #63 , #88 , #91 , #93 , #95 , #105 , #116 , #117 , #128, #130 , #134 , #135 , #143 , #147

Of those 20, only 3 actually found a vulnerability. They are #116 , #128 , and #130 . Edit: And #55. Who wants to help review the rest?

[kadenzipfel]

To add to @jkoppel's escalation, this vulnerability specifically references the ability to frontrun _distribute with a re-registration, allowing the attacker to steal funds. The lack of authorization validation in setPoolActive is simply a dependency of the vulnerability. It's imperative that we separate "lack of setPoolActive authorization" submissions based on the actual impact. The other impact present from this lack of authorizations is griefing as defined in https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/356 and should be classified separately.

[rvierdiiev]

I am not going to take any side in this escalation, however in order to protect myself from possible results will say, that my issue #55 also stated about ability to change bid amount with frontrunning

Another usage of this will be when distribute for milestone will be called to frontrun it to set pool to active, then change recipient bid to bigger amount, then again set pool to inactive and receive bigger payment.

[0x00ffDa]

I agree with jkoppel and kadenzipfel.

The other impact present from this lack of authorizations is griefing as defined in #356 and should be classified separately.

If this ends up getting split according to attack method, #574 is also a griefing / DoS attack but more specific.

[jkoppel]

The griefing attacks do not work. They require front-running every transaction, and, if it starts getting annoying, users can just start prefixing their calls with setPoolActive().

[Abelaby]

Following @rvierdiyev example I want to drop in my issue #597 I have stated multiple potential issues that could arise from this lack of access modifier including the frontrun issue.

[jkoppel]

@Abelaby Where do you mention the use of frontrunning to steal funds? I only see the griefing attack in your writeup.

[Abelaby]

@jkoppel oh mb, I was referring to the DoS caused by front running in my case.

On another note, referring to your comment above;

The griefing attacks do not work. They require front-running every transaction, and, if it starts getting annoying, users can just start prefixing their calls with setPoolActive().

I believe griefing attacks are very much possible, the malicious user can just listen for specific transactions and front run them with setPoolActive(). Even if gets annoying, the 'average' user might just not be aware of or will start prefixing their calls with setPoolActive().

And hypothetically, if the attacker is dedicated enough, their script can be smart enough to just not frontrun transactions with prefixing setPoolActive() so that the users prefixing their calls with setPoolActive() will just sabotage themselves.

And frontrunning every transactions are very much possible in chains with low cost, making this a valid attack vector.

[jkoppel]

I remain in favor of classifying the griefing attacks as a Low, but have nothing factual to add.

I've gone through the same set of 20 reports. In addition to the 4 that mention the High attack, another 2 mention the griefing attack: #63 and #88. There is an argument that #95 mentions it, but you have to squint hard.

If the griefing attack is classified as a Medium, then Sherlock rules state that such reports should be kept as a duplicate of this one.

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.
• In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

[neeksec]

Agree with Escalation.

The Sherlock docs are clear in this senario.

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.
• In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low. Scenario B: In the above example if the root issue A is one of the following generic vulnerabilities:
• Reentrancy
• Access control
• Front-running Then the submissions with valid attack paths and higher vulnerability are considered valid. If the submission is vague or does not identify the attack path with higher severity clearly it will be considered low.
• B is a valid issue
• C is low

DoS/griefing is low in this case.

Valid duplicates,

055, #116, #128, #130, #151, #188, #246, #255, #320, #450

Other submissions should be put low severity.

Comment if I mis-picked or missed any.

[osmanozdemir1]

If the thing that matters is not the bug itself (setPoolActive being external) but the impact of the bug and how the bug can be used (front-running and increasing the proposalBid), then issues should be duped according to these impact not the bug itself.

All of the issues mentioned above describes front-running and increasing the proposalBid. That's the impact of the bug. That's why only these issues rewarded, right? Only validating these issues means the actual problem is not setPoolActive being external, it is the proposalBid being front-runned. But that is already a confirmed issue. #497

If the important thing is the impact, it doesn't matter which function is being front-runned. _allocate or _distribute or _registerRecipient. What's the difference? The actual attack is front-running and increasing the proposalBid. If only these 11 issues going to be validated, they should be duped with #497

[jkoppel]

@osmanozdemir1

The Sherlock docs are very clear about this. To qualify, a report must find both the root cause and any Medium or High attack path, and then the reports are grouped by root cause.

Yes, there is a second attack that also involves front-running that transaction, but it works differently. A set of transactions that exploits this issue will not exploit that issue, nor vice versa. A fix to this issue will not fix that one, nor vice versa.

Not in Sherlock docs, but a question I like to ask when thinking about dups: if the sponsor had only gotten this report, and was committed to only fixing medium and high vulnerabilities, would that be sufficient? If they had only gotten #497 and not this or its dupes, then the issue would be at large in the code.

[jacksanford1]

https://github.com/allo-protocol/allo-v2/pull/340

[osmanozdemir1]

@osmanozdemir1

The Sherlock docs are very clear about this. To qualify, a report must find both the root cause and any Medium or High attack path, and then the reports are grouped by root cause.

Yes, there is a second attack that also involves front-running that transaction, but it works differently. A set of transactions that exploits this issue will not exploit that issue, nor vice versa. A fix to this issue will not fix that one, nor vice versa.

Not in Sherlock docs, but a question I like to ask when thinking about dups: if the sponsor had only gotten this report, and was committed to only fixing medium and high vulnerabilities, would that be sufficient? If they had only gotten #497 and not this or its dupes, then the issue would be at large in the code.

A similar question can be ask this way: If the protocol had only gotten a few setPoolActive reports, but none of these reports mentioned any high/medium scenario, wouldn't the protocol fix this issue? They would definitely fix it because the bug itself is clear like a blue sky.

But it's okay, I get your point and rules are the rules. It is a good lesson for me and for some others :)

[Evert0x]

Planning to accept escalation and remove all duplicates except

https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/55, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/116, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/128, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/130, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/151, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/188, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/246, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/255, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/320, https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/450

[Evert0x]

Result: High Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• jkoppel: accepted

[jack-the-pug]

Fixed.