SBSecurity - 0 amount will be distributed if `recipient.proposalBid` is very low

[sherlock-admin2]

SBSecurity

medium

0 amount will be distributed if recipient.proposalBid is very low

No amount will be distributed if recipient.proposalBid is very low and always round down to 0.

Vulnerability Detail

In RFPSimpleStrategy.sol, the distribution logic involves using milestones by which the pool amount is divided by the milestone.amountPercentage.

The exact line of code:

// Calculate the amount to be distributed for the milestone
uint256 amount = (recipient.proposalBid * milestone.amountPercentage) / 1e18;

The strategy can have multiple milestones, but their amountPercentage sum must add up to 1e18.

So the scenarios would be to have one with amountPercentage = 1e18 or multiples below 1e18 that add up to 1e18.

Let's look at a practical example:

1. User is registered with proposalBid = 15

2. Pool manager allocated him and he became acceptedRecipientId.

3. Pool manger set milestones (3 milestones).

   First with amountPercentage = 5% = 5e16 = 50,000,000,000,000,000

   Second with amountPercentage = 50% = 50e17 = 500,000,000,000,000,000

   Third with amountPercentage = 45% = 4.5e17 = 450,000,000,000,000,000

4. acceptedRecipientId calls submitUpcomingMilestone()

5. Pool manager calls _distribute() and when the line of code above comes,

   15 * 50,000,000,000,000,000 = 750,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

Impact

Take the example above and consider that if we have 20 milestones with amountPercentage = 5%, if all milestones go through distribute() no amount will actually be distributed.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is difficult to give an exact recommendation because there are many things that this depends on and cannot be sure, but it is nice to have a minimum value of the parameter or to change the percentage performance at all.

[sherlock-admin]

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

n33k commented:

low

[asendz]

Escalate

Don’t agree with the severity since there is another possible scenario: when amountPercentage is relatively small percentage 0.05% for example and proposalBid is less than 200. It can be the case when there are a lot of milestones for project which wants to have consistent funding throughout the time and given the fact that these 2 variables are set by different actors (poolManager and recipient) makes the scenario even more possible.

1. User is registered with proposalBid = 110

2. Pool manager allocated him and he became acceptedRecipientId.

3. Pool manger set milestones (50 milestones - long term funding). Last one’s **amountPercentage** = 60%

   **amountPercentage** of the rest milestones has to be set to 0.83% in order to be equal

4. acceptedRecipientId calls submitUpcomingMilestone()

5. Pool manager calls _distribute() and when the line of code above comes,

   110 * 8,300,000,000,000,000 = 913,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

6. Then, if all these milestones pass, the last one will receive its value, and the remaining value will not be allocated to the allocated recipient, only the pool manager can withdraw it for him, which is not the purpose of the protocol.

[sherlock-admin2]

Escalate

Don’t agree with the severity since there is another possible scenario: when amountPercentage is relatively small percentage 0.05% for example and proposalBid is less than 200. It can be the case when there are a lot of milestones for project which wants to have consistent funding throughout the time and given the fact that these 2 variables are set by different actors (poolManager and recipient) makes the scenario even more possible.

1. User is registered with proposalBid = 110

2. Pool manager allocated him and he became acceptedRecipientId.

3. Pool manger set milestones (50 milestones - long term funding). Last one’s **amountPercentage** = 60%

   **amountPercentage** of the rest milestones has to be set to 0.83% in order to be equal

4. acceptedRecipientId calls submitUpcomingMilestone()

5. Pool manager calls _distribute() and when the line of code above comes,

   110 * 8,300,000,000,000,000 = 913,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

6. Then, if all these milestones pass, the last one will receive its value, and the remaining value will not be allocated to the allocated recipient, only the pool manager can withdraw it for him, which is not the purpose of the protocol.

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]

Escalate

15 * 50,000,000,000,000,000 = 750,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

110 * 8,300,000,000,000,000 = 913,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

Should be low severity, In both the examples given above and in the submission, the amount lossed is so small that it does not qualify for medium severity.

First example shows a loss of 0.75 amount of token Second example shows a loss of 0.913 amount of token

Note: This is not mentioned in the submission, but even with a 2 decimal precision token which afaik is the lowest existing decimal token GUSD, the loss will amount to 0.075 USD and 0.0913 USD respectively. Not sure if sherlock supports additional information during escalation period, but unless watson can find a token example that can prove a significant value of loss in tokens, this finding should be low severity.

[sherlock-admin2]

Escalate

15 * 50,000,000,000,000,000 = 750,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

110 * 8,300,000,000,000,000 = 913,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

Should be low severity, In both the examples given above and in the submission, the amount lossed is so small that it does not qualify for medium severity.

First example shows a loss of 0.75 amount of token Second example shows a loss of 0.913 amount of token

Note: This is not mentioned in the submission, but even with a 2 decimal precision token which afaik is the lowest existing decimal token GUSD, the loss will amount to 0.075 USD and 0.0913 USD respectively. Not sure if sherlock supports additional information during escalation period, but unless watson can find a token example that can prove a significant value of loss in tokens, this finding should be low severity.

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.

[Slavchew]

Escalate

15 * 50,000,000,000,000,000 = 750,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

110 * 8,300,000,000,000,000 = 913,000,000,000,000,000 / 1e18 will result in 0 in Solidity and the distributed amount will be 0.

Should be low severity, In both the examples given above and in the submission, the amount lossed is so small that it does not qualify for medium severity.

First example shows a loss of 0.75 amount of token Second example shows a loss of 0.913 amount of token

Note: This is not mentioned in the submission, but even with a 2 decimal precision token which afaik is the lowest existing decimal token GUSD, the loss will amount to 0.075 USD and 0.0913 USD respectively. Not sure if sherlock supports additional information during escalation period, but unless watson can find a token example that can prove a significant value of loss in tokens, this finding should be low severity.

Yes, the second example shows a loss of 0.913 amount of token, but for one single milestone, consider the example where there are 50 milestones, 49 of them will lose this amount, and also if there are many milestones, with a low percentage none of them will distribute tokens to acceptedRecipientId which defeats the purpose of the contract because in this scenario only the pool manager will be able to withdraw them.

[neeksec]

Suggest to keep low severity.

Low amount of fund lost in pool because of division. And this happens under rare cases.

[Evert0x]

Agree with lead judge. Even in the most extreme scenario the rounding loss is not significant.

Planning to reject escalation and keep issue state as is.

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted
• asendz: rejected