sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

feelereth - Skipping rewards accounting for couriers does reduce the effective rewards rate for other users #147

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

feelereth

medium

Skipping rewards accounting for couriers does reduce the effective rewards rate for other users

Summary

Rewards accounting is skipped for couriers. This reduces the effective rewards rate for other users.

Vulnerability Detail

The lack of proper rewards accounting for couriers does reduce the effective rewards rate for other users. Here is a detailed explanation: The key code sections are:

  1. In _mint():

       // Skip rewards update on the courier. This means accounting isn't
       // accurate for them, so they *should not* be allowed to claim rewards. This
       // slightly reduces the effective overall rewards rate.
  2. In _burn():

       // NOTE: We skip rewards update on the courier. This means accounting isn't
       // accurate for them, so they *should not* be allowed to claim rewards. This
       // slightly reduces the effective overall rewards rate.
  3. In _transfer():

       Rewards.updateUserState(s, a, from, data % Q112);
    
       //...
    
       Rewards.updateUserState(s, a, to, data % Q112);

The issue is that rewards accounting is properly tracked for normal users via the Rewards.updateUserState() calls in _transfer(). However, for couriers, the rewards accounting is skipped in _mint() and _burn(). This means that as fees are transferred from users to couriers, the rewards owed to couriers are not properly tracked. Over time, this results in the total rewards owed diverging from the total rewards being tracked and paid out. Specifically, rewards owed will be higher than tracked rewards. Since the total rewards rate and payouts are based on the tracked rewards, this effectively reduces the rewards rate for other users.

Impact

It reduces the effective rewards rate for other users. Over time, as fees are transferred from users to couriers, the rewards owed to couriers will diverge from the rewards being tracked and paid out. Specifically, rewards owed will be higher than tracked rewards.

Since the total rewards rate and payouts are based on the tracked rewards, skipping accounting for couriers means the effective rewards rate will be lower than the configured rate.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L517-L519 https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L415-L421

Tool used

Manual Review

Recommendation

Rewards accounting should also be done for couriers. A suggestive example:

In _mint():

   // Track rewards for courier
   Rewards.updateUserState(s, a, courier, 0);

In _burn():

   // Track rewards for courier
   Rewards.updateUserState(s, a, courier, balances[courier] % Q112);

This ensures proper tracking of rewards owed to couriers, maintaining the effective rewards rate for all users.

Duplicate of #115

sherlock-admin2 commented 1 year ago

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

panprog commented:

invalid, because lack of updateUserState doesn't influence overall reward rate. And overall pool state (updatePoolState) is correct, because transferring some shares to couriers doesn't change totalSupply

MohammedRizwan commented:

invalid issue seems intended design