WIchiFarm#pendingRewards suffers from significant precision loss causing loss of rewards

Summary

IchI LPs are 18 dp tokens while IchiPerShare is only 9 dp. In conjunction with how small typical Ichi LP values are, the precision loss caused during calculation can cause nontrivial loss to users.

Vulnerability Detail

WIchiFarm.sol#L122-L127

    (uint256 enIchiPerShare, , ) = ichiFarm.poolInfo(pid);
    uint256 stIchi = (stIchiPerShare * amount).divCeil(10 ** lpDecimals);
    uint256 enIchi = (enIchiPerShare * amount) / (10 ** lpDecimals); <- @audit-issue precision loss here
    uint256 ichiRewards = enIchi > stIchi ? enIchi - stIchi : 0;
    // Convert rewards to ICHI(v2) => ICHI v1 decimal: 9, ICHI v2 Decimal: 18
    ichiRewards *= 1e9;

Since stIchi and enIchi are calculated separate from eachother, it results in precision loss. Normally this precision loss would result in trivial losses but in these circumstances the losses could be quite large. This is because IchiPerShare is stored as a 9 dp value. Additionally even large deposits result in very low LP values. This creates a scenario where users can lose substantial rewards to precision loss.

Example: A user deposits $500 worth of ICHI to get ICHI LP. This deposit results in the user receiving ~860000000 LP (based on current conditions). Now imagine a the IchiPerShare increases by 1e9 (1 unit of IchIV1). Based on the current math this would result in the user getting 0 in rewards:

860000000 * 1e9 / 1e18 = 0.86 which is truncated to 0.

Impact

Precision loss will cause permanent loss to the user

Code Snippet

WIchiFarm.sol#L110-L133

Tool used

Manual Review

Recommendation

Calculate rewards like WConvexPools to reduce precision loss as much as possible.

IAm0x52 commented 1 year ago

Your comment would be true if a single user made up the entire pool. However the pool is an aggregate of many users that all earn rewards as a whole (all LP deposited to the same address). It would be true that each user may not accumulate more than the minimum individually but as a collective they would exceed it and therefore should be due their fair share.

IAm0x52 commented 1 year ago

If there are two users and they deposit 500000000 LP each. Each user will be due:

500000000 * 1e9 / 1e18 = 0.5 => 0

Together there will be 1000000000 LP so as a whole they will earn from the ICHI farm:

1000000000 * 1e9 / 1e18 = 1

If each user were to deposit independently, neither would receive a reward form the farm (both truncate to 0) but when put in together they earn 1 which isn't truncated. Therefore in that case both users should be due their fair share

securitygrid commented 1 year ago

Escalate for 10 USDC As mentioned by sponsor, the impact of this issue does not reach H. It's valid M.

sherlock-admin commented 1 year ago

Escalate for 10 USDC As mentioned by sponsor, the impact of this issue does not reach H. It's valid M.

You've created a valid escalation for 10 USDC!

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.

ctf-sec commented 1 year ago

Based on the comment above, this issue can be a valid medium

hrishibhat commented 1 year ago

Escalation accepted

Valid low After further discussions and review of the comments, the impact of the issue is not significant enough for a valid medium. The losses here are of the order 9.9e8 loss out of a 18 dp token.