Potential Distribution Failure Due to Zero Rounded Transfer Amount

[codehawks-bot]

Potential Distribution Failure Due to Zero Rounded Transfer Amount

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L145-L151

Summary

The _distribute function in the smart contract calculates the amount to distribute to each winner based on their respective percentages. However, for tokens that do not allow transfers of zero amount, and in cases where the calculated amount rounds down to zero, the distribution will fail.

Vulnerability Details

The vulnerability arises from the calculation of the distribution amount using the formula totalAmount * percentages[i] / BASIS_POINTS. If a winner's percentage is small, or if the totalAmount of tokens to be distributed is low, the calculated amount can round down to zero. Many token contracts do not allow transfers of zero amount, considering them as no-ops or even reverting the transaction. As a result, when the _distribute function attempts to transfer a zero amount, the token's safeTransfer function can revert, causing the entire distribution process to halt.

Impact

If any winner's calculated distribution amount rounds to zero, the entire distribution process will fail.

Tools Used

Manual Review

Recommendations

Before executing the safeTransfer function, check if the calculated amount is greater than zero. If the amount is zero, skip the transfer for that particular winner and continue with the next one.

[PatrickAlphaC]

Moving to low. I'd need a PoC to see it actually cause a failure. As written this has a low likelihood of happening with low impact.

[leeftk]

Our report included a POC as well as showing the impact of this issue. I'd argue that this issue should be a medium considering that there isn't a cap on the minimum amount of tokens that you have to fund a contest. Considering we weren't giving much context into what these "contests" actually are, it is safe to assume we can have a contest with a very low reward amount (winners can a reward for watching a 15-second ad for example) in this case at 10,000 basis points if the percentage is 1 basis point and the amount of tokens per use is 999 WEI this issue could still be valid. While 999 WEI seems irrelevant if we assume an UPONLY case for ETH over the next few decades this amount could actually be an impactful loss to the user. Plus considering as per the docs, once funds are sent to the contract they are not to be refunded to the Sponsor these funds would get stuck in the contract leaving them completely at risk.

https://github.com/Cyfrin/2023-08-sparkn/issues/464

[serial-coder]

Our report included a POC as well as showing the impact of this issue. I'd argue that this issue should be a medium considering that there isn't a cap on the minimum amount of tokens that you have to fund a contest. Considering we weren't giving much context into what these "contests" actually are, it is safe to assume we can have a contest with a very low reward amount (winners can a reward for watching a 15-second ad for example) in this case at 10,000 basis points if the percentage is 1 basis point and the amount of tokens per use is 999 WEI this issue could still be valid. While 999 WEI seems irrelevant if we assume an UPONLY case for ETH over the next few decades this amount could actually be an impactful loss to the user. Plus considering as per the docs, once funds are sent to the contract they are not to be refunded to the Sponsor these funds would get stuck in the contract leaving them completely at risk.

464

Escalate

This issue is invalid.

First, the BASIS_POINTS is a constant of 10000.

Second, the whitelisted tokens are the only primary stablecoin tokens (confirmed by the developer), such as JPYC (18 decimals), USDT (6 decimals), USDC (6 decimals), DAI (18 decimals), etc.

Suppose the USDC (6 decimals) is used as a prize token. For instance, 1 USDC (1 * 10^6 = 1000000 wei) and let's say percentages[I] = 1 (0.01%), so:

amount = totalAmount * percentages[i] / BASIS_POINTS;
       = 1000000 * 1 / 10000;  //@audit -- percentages[I]: 1 == 0.01%
       = 100 //@audit -- 100 wei will be transferred

As you can see, the result is still correct. Furthermore, the PoC by leeftk was incorrect since he is trying to send 10 wei as the totalAmount param, which is impractical. He did not account for the token decimal in the PoC.

For this reason, this issue is invalid.

[leeftk]

Our report included a POC as well as showing the impact of this issue. I'd argue that this issue should be a medium considering that there isn't a cap on the minimum amount of tokens that you have to fund a contest. Considering we weren't giving much context into what these "contests" actually are, it is safe to assume we can have a contest with a very low reward amount (winners can a reward for watching a 15-second ad for example) in this case at 10,000 basis points if the percentage is 1 basis point and the amount of tokens per use is 999 WEI this issue could still be valid. While 999 WEI seems irrelevant if we assume an UPONLY case for ETH over the next few decades this amount could actually be an impactful loss to the user. Plus considering as per the docs, once funds are sent to the contract they are not to be refunded to the Sponsor these funds would get stuck in the contract leaving them completely at risk.

464

Escalate

This issue is invalid.

First, the BASIS_POINTS is a constant of 10000.

Second, the whitelisted tokens are the only primary stablecoin tokens (confirmed by the developer), such as JPYC (18 decimals), USDT (6 decimals), USDC (6 decimals), DAI (18 decimals), etc.

Suppose the USDC (6 decimals) is used as a prize token. For instance, 1 USDC (1 * 10^6 = 1000000 wei) and let's say percentages[I] = 1 (0.01%), so:

amount = totalAmount * percentages[i] / BASIS_POINTS;
       = 1000000 * 1 / 10000;  //@audit -- percentages[I]: 1 == 0.01%
       = 100 //@audit -- 0 wei will be transferred

As you can see, the result is still correct. Furthermore, the PoC by leeftk was incorrect since he is trying to send 10 wei as the totalAmount param, which is impractical. He did not account for the token decimal in the PoC.

For this reason, this issue is invalid.

"Impractical" is subjective. Here are 10 different issues all from code4rena and sherlock that are the same as this issue. The majority of them are labeled as High some of them are medium. Some of which have rounding issues because a small number of tokens was passed as the 'amount' to a function. This issue of precision loss due to small amounts is extremely common and widely known.

Also to dispute your point GUSD (Gemini dollar) is a stable coin that only uses 2 decimal places. So if I were to copy the example you have above

Suppose the GUSD (2 decimals) is used as a prize token. For instance, 1 USDC (1 * 10^2 = 100 wei) and let's say percentages[I] = 1 (0.01%), so:

amount = totalAmount * percentages[i] / BASIS_POINTS;
       = 100 * 1 / 10000;  //@audit -- percentages[I]: 1 == 0.01%
       = 0.01 = 0  //@audit -- 100 wei will be transferred in this case even 

// even amounts up to 999 WEI in GUSD or $9.99 would cause precision loss. 
//Which is significant loss to the protocol and the users themselves. 
//The sponsor would have to top up the contract with more tokens in order to make the value above 999.
//Leaving them having to use more funds than expected and pay extra money in gas costs to do this. 
//Hence not so "impractical".

Since it was not made clear which stablecoins will be whitelisted it is safe to assume GUSD could be used or a new stablecoin in the future could be created (PYUSD?) that uses 2 decimals or less causing winners to not be able to receive their tokens.

Here are the references to the similar issues that I stated previously.

1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

[serial-coder]

Let me explain further about the word "impractical".

I argue your statement below.

Suppose the GUSD (2 decimals) is used as a prize token. For instance, 1 USDC (1 * 10^2 = 100 wei) and let's say percentages[I] = 1 (0.01%), so:

amount = totalAmount * percentages[i] / BASIS_POINTS;
       = 100 * 1 / 10000;  //@audit -- percentages[I]: 1 == 0.01%
       = 0.01 = 0  //@audit -- 100 wei will be transferred in this case even 

// even amounts up to 999 WEI in GUSD or $9.99 would cause precision loss. 
//Which is significant loss to the protocol and the users themselves. 
//The sponsor would have to top up the contract with more tokens in order to make the value above 999.
//Leaving them having to use more funds than expected and pay extra money in gas costs to do this. 
//Hence not so "impractical".

Applying the above settings means you are sending 0.01% of 1 GUSD ($1) to a winner. This is what the word "impractical" I was talking to. Sending 0.01% of $1 (= $0.0001), is that a "practical" scenario?

Let's discuss the case of 999 WEI ($9.99) you were talking about. Sending 0.01% of $9.99 (= $0.000999), is that still a "practical" scenario for you?

[leeftk]

LOL seems like you're just trying to argue and I don't have time for that but in response to your snarky response here's another possibility.

amount = totalAmount * percentages[i] / BASIS_POINTS;
       = 999 * 10 / 10000;
       = 0.999  = 0

But you explicitly chose to disregard the mention of the issues I mentioned, and the fact that in the future there could be more stable coins with different decimal amounts. It is clear you just want to go back and forth which I don't have time to do so I'll state this as my last piece and let the judges decide from here :)

What is practical or impractical is up to the user to decide. Even if a protocol loses a user 1 cent, that goes against what it's supposed to do. So in this case impractical is completely subjective as I've already stated. But if you check the issues I sent you'll see the "impractical" amounts led to multiple highs and mediums. But my guess is you didn't take the time to look at them before leaving this response. I try not to add tone to text but from the looks of it, my response really pissed you off haha. So like I said I'll keep it simple and this will be my last reply to YOU and instead of going back and forth I'll let the judges decide based on the information we both provided. Feel free to check the issues I sent you which are all valid issues with similar scenarios that are all medium or higher. :)

[PatrickAlphaC]

Hi all, thanks for the discussion.

@leeftk please remember to keep the discussion on the topic, and refrain from calling other security researchers names such as "snarky". Stay on the course of arguing the issue, not the other person!

But my guess is you didn't take the time to look at them before leaving this response.

Additionally, comments like this which are aimed at the security researcher and not the issue have no merit here. Please focus on the security of the protocol, not ad hominem.

I agree with @leeftk that this is an issue. However, I'm not convinced the impact or the likelihood are high. Impact might be medium, likelihood is low. And for that, I'm going to mark as low.

[PatrickAlphaC]

I also agree that you have a PoC and are therefore the selected finding.