sherlock-admin commented 1 year ago

jkoppel

medium

QV strategy cannot receive native token

The RFP and DonationMerkle strategies have a receive() method so that they can be funded with native token. The QV strategy does not, and therefore it is incompatible with the native token.

Vulnerability Detail

See summary

Impact

Cannot use QV strategy with native token.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/qv-base/QVBaseStrategy.sol#L574

Notice the lack of a receive() method, which is also lacking in QVSimpleStrategy. In contrast, the others have it, e.g.: https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L500 .

Allo.fundPool calls _transferAmountFrom: https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/core/Allo.sol#L516 . Tracing the definitions, it invokes the strategy with call(gas(), to, amount, gas(), 0x00, gas(), 0x00) (in SafeTransferLib). I.e.: it invokes it with empty calldata, and will therefore try to call receive() and revert.

Tool used

Manual Review

Recommendation

Add a receive() method.

sherlock-admin commented 1 year ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

low, no fund lose, only incompatible with NATIVE token, should consider to fix

jkoppel commented 12 months ago

Escalate.

Actually, there can be fund loss. If you attempt to create a pool but do not grant it initial funds, then you'll have paid the baseFee for pool creation but will have an inoperable pool. Further, Sherlock rules do consider valid issues that prevent contracts from achieving their purpose, even in the absence of fund loss.

sherlock-admin2 commented 12 months ago

Escalate.

Actually, there can be fund loss. If you attempt to create a pool but do not grant it initial funds, then you'll have paid the baseFee for pool creation but will have an inoperable pool. Further, Sherlock rules do consider valid issues that prevent contracts from achieving their purpose, even in the absence of fund loss.

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.

nevillehuang commented 12 months ago

Escalate.

Actually, there can be fund loss. If you attempt to create a pool but do not grant it initial funds, then you'll have paid the baseFee for pool creation but will have an inoperable pool. Further, Sherlock rules do consider valid issues that prevent contracts from achieving their purpose, even in the absence of fund loss.

If it reverts due to lack of receive payable, doesn't it mean pool creator get back the fees they paid?

But i do agree that this breaks core functionality since it is explicitly stated by protocol that all tokens are supported, even native ETH which is so widely used. I am inclined to think this is medium severity since i am very sure the protocol does not intend to not support native ETH for funding, which can potentially cause loss of fees.

Sidenote: #27, #299, #363, #506, #521, #541, #549, #595, #624, #677, #894, #941, #957 are valid duplicates of this issue.

jkoppel commented 12 months ago

Nice work finding the duplicates!

If it reverts due to lack of receive payable, doesn't it mean pool creator get back the fees they paid?

Only if you try to fund the pool at creation time.

0x00ffDa commented 12 months ago

To qualify for Medium, I think it depends whether the fees lost would constitute "a material amount of funds". Also, the protocol owner can refund the fee to the user, so arguably not lost at all.

For me, I didn't report this because, although it's a valid issue ... I expected it to be judged Low. It will be interesting to see what they decide.

thelostone-mc commented 11 months ago

Would agree that this is an issue which needs to be fixed

neeksec commented 11 months ago

Suggest to keep make this low/info.

The fees lost is not "a material amount of funds" and could be refunded by Allo team.

MLON33 commented 11 months ago

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

Evert0x commented 11 months ago

I believe this falls into the same category as https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/862#issuecomment-1777391236

https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue

Breaks core contract functionality, rendering the contract useless

Planning to accept and make medium, but will get back to this.

Evert0x commented 11 months ago

But i do agree that this breaks core functionality since it is explicitly stated by protocol that all tokens are supported, even native ETH

@nevillehuang where is this stated?

nevillehuang commented 11 months ago

But i do agree that this breaks core functionality since it is explicitly stated by protocol that all tokens are supported, even native ETH

@nevillehuang where is this stated?

In their docs here, which is part of the contest details.

Pools can also be funded with either native Ether or an ERC20 token. Note though that a pool can only distribute one token, which is determined when the pool is created. Trying to call fundPool with a token other than the one used when the pool was created will cause the method to revert with an error. Pools should always be funded using the fundPool method and you should never transfer funds directly to Allo.sol

Evert0x commented 11 months ago

Based on the language in their docs it's clear that receiving native Ether is a core functionality of the protocol.

Evert0x commented 11 months ago

Result: Medium Has Duplicates

sherlock-admin2 commented 11 months ago

Escalations have been resolved successfully!

Escalation status:

jkoppel: accepted

sherlock-audit / 2023-09-Gitcoin-judging

jkoppel - QV strategy cannot receive native token #256

QV strategy cannot receive native token

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation