jkoppel - RFPSimpleStrategy.allocate() can be front-run, allowing the someone to bid very low but charge very high

[sherlock-admin]

jkoppel

high

RFPSimpleStrategy.allocate() can be front-run, allowing the someone to bid very low but charge very high

The winning recipient of an RFP pool may change their bid right at the moment they're chosen, allowing them to get paid for more than the pool-owner expects.

There is another vulnerability where the recipient can abuse the lack of access control on setPoolActive() to do this . This is a separate issue, as it involves front-running allocate() instead of distribute(), and the fix to that issue (fixing access control on setPoolActive()) will not fix this, nor vice-versa.

Vulnerability Detail

1. Alice creates a pool and funds it to 100K USDC
2. Bob submits the winning bid, saying he can do it for only 500 USDC. (I.e.: he calls registerRecipient() with a bid of 500 USDC)
3. Alice calls allocate() to award the contract to Bob....
4. ...but Bob front-runs it by calling registerRecipient again, changing his bid to 100k USDC.
5. Now Bob won with a bid of 100k USDC. Either Alice notices the change before the first call to distribute(), in which case she withdraws from the pool and loses the (quite substantial) fees used to fund it, or she doesn't, and pays way too much

There is one very realistic use-case that makes this even worse: Many projects require some up-front payment. For such a project, the pool owner may both allocate() to the winner and distribute() the first milestone in a single transaction. In that case, there is no chance for the pool owner to detect Alice

Similarly, because Bob's bid is so low, he may be able to socially-engineer Alice to pay the whole thing immediately. E.g.: "Hi, I'm Bob, an expert at your task. I really believe in what you're doing, so I'm willing to do it for only $100. All I ask is that I get paid up-front immediately, so that I can treat my wife to a nice dinner for her birthday tomorrow."

Impact

Bidders can bait-and-switch to win a bid with the highest possible bid amount, even if there are lower bidders. And they can trick pool owners into paying them far more than expected.

Code Snippet

Notice how registerRecipient() can be called any time until allocate() is called.

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

Tool used

Manual Review

Recommendation

Add slippage protection to allocate(), where the call to allocate() accepts a parameter for the expected bid, and reverts if it has changed.

Duplicate of #497

[jkoppel]

Escalate.

This is not a duplicate of #458. This is a duplicate of #497. In fact, the second paragraph of this submission explicitly says this is not a duplicate of #458.

[sherlock-admin2]

Escalate.

This is not a duplicate of #458. This is a duplicate of #497. In fact, the second paragraph of this submission explicitly says this is not a duplicate of #458.

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.

[neeksec]

Agree that this is a duplicate of #497.

[Evert0x]

Planning to accept escalation and duplicate with #497

[Evert0x]

Result: High Duplicate of #497

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• jkoppel: accepted