0x52 - amount_claimable_per_share accounting is broken and will result in vault insolvency

[github-actions]

0x52

high

amount_claimable_per_share accounting is broken and will result in vault insolvency

Summary

Claim accounting is incorrect if there is a deposit when amount_claimable_per_share !=0, because position.amount_claimed isn't initialized when the deposit is created.

Vulnerability Detail

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

When calculating the amount of WETH to claim for a user, the contract simply multiplies the share count by the current amount_claimable_per_share and then subtracts the amount that has already been paid out to that token holder. This is problematic for deposits that happen when amount_claimable_per_share != 0 because they will be eligible to claim WETH immediately as if they had been part of the vault since amount_claimable_per_share != 0.

Example;
User A deposits 1 ETH and receives 1 share. Over time the loan pays itself back and claim is able to withdraw 0.1 WETH. This causes amount_claimable_per_share = 0.1. Now User B deposits 1 ETH and receives 1 share. They can immediately call claim which yields them 0.1 WETH (1 * 0.1 - 0). This causes the contract to over-commit the amount of WETH to payout since it now owes a total of 0.2 WETH (0.1 ETH to each depositor) but only has 0.1 WETH.

Impact

Contract will become insolvent

Code Snippet

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

Tool used

Manual Review

Recommendation

position.amountClaimed needs to be initialized when a deposit is made:

    # deposit WETH to Alchemix
    shares_issued: uint256 = self._deposit_to_alchemist(_amount)
    position.shares_owned += shares_issued
+   position.amount_claimed += shares_issued * amount_claimable_per_share
    self.total_shares += shares_issued
    
    self.positions[_token_id] = position

[HickupHH3]

Dup of #114

[IAm0x52]

Escalate for 1 USDC

Not a dupe of #114. That focuses on depositing twice to the same token (which can't happen outside of admin abuse). The issue is that it generally applies to all deposits. Please re-read my example as to why this is an issue and how it leads to gross over-commitment of rewards.

Two reasons I disagree with this being low:

1. The argument that this will only be used for a short period and so "it doesn't have much impact" is a poor argument. This is meant as a general utility that anyone can use and they should be able to make a funding period as long as they want
2. This is a serious issue that will lead to rewards being over-committed and the vault WILL go insolvent as a result. The extra fees being paid will be taken from other users and will GUARANTEED cause loss of funds to other users.

Would like to add that this and #113 are the same issue and escalations should be resolved together.

[sherlock-admin]

Escalate for 1 USDC

Not a dupe of #114. That focuses on depositing twice to the same token (which can't happen outside of admin abuse). The issue is that it generally applies to all deposits. Please re-read my example as to why this is an issue and how it leads to gross over-commitment of rewards.

Two reasons I disagree with this being low:

1. The argument that this will only be used for a short period and so "it doesn't have much impact" is a poor argument. This is meant as a general utility that anyone can use and they should be able to make a funding period as long as they want
2. This is a serious issue that will lead to rewards being over-committed and the vault WILL go insolvent as a result. The extra fees being paid will be taken from other users and will GUARANTEED cause loss of funds to other users.

Would like to add that this and #113 are the same issue and escalations should be resolved together.

You've created a valid escalation for 1 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]

Agree, duplicate of #113 and will fix.

[hrishibhat]

Escalation accepted

Considering this & its duplicate #113 as valid issues

[sherlock-admin]

Escalation accepted

Considering this & its duplicate #113 as valid issues

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[Unstoppable-DeFi]

Unstoppable-DeFi/fair-funding#8

[HickupHH3]

Fix is ok: total_claimable is split amongst newly added shares as well (see issue #50), but I take it that it is an acceptable feature based on the resolution of the referenced issue.

I'd also like to highlight that some yield can be lost due to rounding, see example below.

Example

total_shares = 0.49e18
Assuming a yield of 0.0001 ETH has been accumulated and marked claimable so far,
amount_claimable_per_share = 0.0001e18 * 1e6 / 0.49e18 = 204
total_claimable = 204 * 0.49e18 = 9.996e19

Adding new_shares = 1.5e18,
amount_claimable_per_share = 9.996e19 / (0.49e18 + 1.5e18) = 50
total_claimable = 50 * (0.49e18 + 1.5e18) = 9.95e19, which is a discrepancy of (9.996e19 - 9.95e19) / 1e6 = 4.6e-7 ETH.

Probably negligible given low yield accumulation + short auction reasoning. Nevertheless, consider using a higher precision value. Edit: I see Unstoppable-DeFi/fair-funding#2 does just that.

[jacksanford1]

Message from Protocol Team:

It’s resolved by another fix, precision is increased now.

Message from Lead Senior Watson:

yup resolved