Bahurum - Loss of yield due to low precision

[github-actions[bot]]

Bahurum

medium

Loss of yield due to low precision

Summary

In the Vault contract function _mark_as_claimable() the claimable per share will be lower than expected due to loss of precision. This can also be abused by calling withdraw_underlying_to_claim() often.

Vulnerability Detail

In Vault contract at line 419:

    self.amount_claimable_per_share += _amount * PRECISION / self.total_shares 

PRECISION = 1e6 and can cause loss of precision of the claimable amount. For example with a yield of 0.1 % a day means 0,000069444 % a minute which corresponds to 6.9444 e-7, which is smaller precision than 1e6. When withdraw_underlying_to_claim() is called at intervals of 1 minutes no amount claimable is accounted. If it is called at intervals of 10 minutes then the amount claimed is still underestimated by more than 10%.

Impact

If withdraw_underlying_to_claim() is called often enough the amount claimable will be underestimated and as a result the lenders will have to wait a longer time to get their funds back.

Code Snippet

https://github.com/sherlock-audit/2023-02-fair-funding/blob/main/fair-funding/contracts/Vault.vy#L419

Tool used

Manual Review

Recommendation

Remove PRECISION from the contract and use DECIMALS instead, which is 1e18 precision and will avoid precision loss issues.

[Unstoppable-DeFi]

At Alchemix APYs (2-5% max per year) this is unlikely to have any meaningful impact. Will fix anyways.

[Unstoppable-DeFi]

https://github.com/Unstoppable-DeFi/fair-funding/pull/2

[hrishibhat]

Considering this issue a low based on Sponsor comment

[bahurum]

Escalate for 50 USDC.

I believe this has been judged wrongly as a Low, and also that with the APY suggested by the protocol team, this has actually an High impact instead of Medium.

This issue always causes funds to be stuck forever into the Vault contract. The amount of funds stuck can be up to the full yield from Alchemix depending on how often withdraw_underlying_to_claim() is called.

I do not understand the comment from the protocol saying that this won't have a meaningful impact because APY is just 2-5%. The issue gets worse if the APY is lower, as the rounding will cut a larger fraction from the yield accrued when accounting the claimable amount. I had considered optimistically in my submission an APY of 36.5% (0.1%/day with no compounding). With lower APY the impact is even higher. In fact with actual value of Alchemix APY given by the team, this becomes, as shown below and according to Sherlock's judging criteria, a High severity issue.

• There are 50 ETH into the vault (50 total shares)
• APY is 2 %, (0.00548 % / day). This means that the amount self-repayed in Alchemix and withdrawable is 0.00274 ETH/day
• A lender or the protocol itself (or anybody else) calls withdraw_underlying_to_claim()
• After 2 days withdraw_underlying_to_claim() is called again: amount_withdrawn = 2 0.00274 1e18 = 548 1e13 and `_amount PRECISION / self.total_shares= 548 * 1e13 * 1e6 / (50 * 1e18) = 548 / 50 = 10. Note that there is a rounding error of 9.6%, meaning that almost 10% of yield generated will not be accounted as claimable, and will be forever stuck into theVault`

The scenario above of withdraw_underlying_to_claim() being called each 2 days is realistic and yet optimistic. If it is called each 4 hours, one has _amount * PRECISION / self.total_shares = 274 / 6 1e13 1e6 / (50 * 1e18) = 274 / 6 / 50 = 0, and all the yield accrued is stuck into the Vault contract forever.

As you can see, an important part (up to full amount) of yield generated will be lost for sure, and that is why I believe this should be marked as High.

[sherlock-admin]

Escalate for 50 USDC.

I believe this has been judged wrongly as a Low, and also that with the APY suggested by the protocol team, this has actually an High impact instead of Medium.

This issue always causes funds to be stuck forever into the Vault contract. The amount of funds stuck can be up to the full yield from Alchemix depending on how often withdraw_underlying_to_claim() is called.

I do not understand the comment from the protocol saying that this won't have a meaningful impact because APY is just 2-5%. The issue gets worse if the APY is lower, as the rounding will cut a larger fraction from the yield accrued when accounting the claimable amount. I had considered optimistically in my submission an APY of 36.5% (0.1%/day with no compounding). With lower APY the impact is even higher. In fact with actual value of Alchemix APY given by the team, this becomes, as shown below and according to Sherlock's judging criteria, a High severity issue.

• There are 50 ETH into the vault (50 total shares)
• APY is 2 %, (0.00548 % / day). This means that the amount self-repayed in Alchemix and withdrawable is 0.00274 ETH/day
• A lender or the protocol itself (or anybody else) calls withdraw_underlying_to_claim()
• After 2 days withdraw_underlying_to_claim() is called again: amount_withdrawn = 2 0.00274 1e18 = 548 1e13 and `_amount PRECISION / self.total_shares= 548 * 1e13 * 1e6 / (50 * 1e18) = 548 / 50 = 10. Note that there is a rounding error of 9.6%, meaning that almost 10% of yield generated will not be accounted as claimable, and will be forever stuck into theVault`

The scenario above of withdraw_underlying_to_claim() being called each 2 days is realistic and yet optimistic. If it is called each 4 hours, one has _amount * PRECISION / self.total_shares = 274 / 6 1e13 1e6 / (50 * 1e18) = 274 / 6 / 50 = 0, and all the yield accrued is stuck into the Vault contract forever.

As you can see, an important part (up to full amount) of yield generated will be lost for sure, and that is why I believe this should be marked as High.

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[Unstoppable-DeFi]

I do not understand the comment from the protocol saying that this won't have a meaningful impact

Not intimately familiar with the Sherlock impact specifications but this is our reasoning for considering it a generally low impact issue:

1) There is no way to profit from it for an attacker, it is purely a griefing attack 2) It would have to be done continuously over very long periods of time for any meaningful amount to be impacted 3) migrate functionality could be used in such a scenario to mitigate the attack 4) it is economically infeasible given the tx costs on mainnet to uphold this attack for long periods and without profiting from it

Of course we fixed the issue anyways since it was valid.

[bahurum]

Thank you for your response. That makes your previous comment clearer.

I agree that if an attacker wants to abuse this issue, then it will be only a griefing attack, but the issue will occurr naturally even if there is no attacker as you or the lenders call withdraw_underlying_to_claim() to withdraw accrued yield from alchemix. Lenders would want to call that function from time to time to get some of the loan repaid.

Also, currently this cannot be mitigated with migration, since the only way to transfer WETH out of the Vault is to call claim(), and that will not transfer all tokens out as explained in the issue.

[hrishibhat]

Escalation rejected

After further consideration & internal discussion, the conclusion is that:

1. The griefing attack would be highly unfeasible for the outcome of causing a loss of 4% over the year.
2. Apart from the attack, for the user it only makes sense to call the function only occasionally whenever the amount_claimable_per_share is significantly higher than tx costs on mainnet. So the impact would be significantly lower, negligible Considering this issue a low.

[sherlock-admin]

Escalation rejected

After further consideration & internal discussion, the conclusion is that:

1. The griefing attack would be highly unfeasible for the outcome of causing a loss of 4% over the year.
2. Apart from the attack, for the user it only makes sense to call the function only occasionally whenever the amount_claimable_per_share is significantly higher than tx costs on mainnet. So the impact would be significantly lower, negligible Considering this issue a low.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.