ether_sky - The rewards distribution in the NFTPositionManager is unfair

[sherlock-admin4]

ether_sky

Medium

The rewards distribution in the NFTPositionManager is unfair

Summary

In NFTPositionManager, users can deposit or borrow assets and earn rewards in zero. However, the distribution of these rewards is not working correctly.

Vulnerability Detail

Let's consider a pool, P, and an asset, A, with a current liquidity index of 1.5.

• Two users, U1 and U2, each deposit 100 units of A into pool P. (Think of U1 and U2 as token IDs.)
• Let's define assetHash(P, A, false) as H.
• In the _supply function, the balance is 100, since it represents the asset amount, not shares (as seen in line 57).

  
  function _supply(AssetOperationParams memory params) internal nonReentrant {
    pool.supply(params.asset, address(this), params.amount, params.tokenId, params.data);

57:  uint256 balance = pool.getBalance(params.asset, address(this), params.tokenId);   _handleSupplies(address(pool), params.asset, params.tokenId, balance); }

- The `shares` for these users would be calculated as `100 ÷ 1.5 = 66.67 shares` each in the `P`.

Now, in the `_handleSupplies` function, we compute the `total supply` and `balances` for these users for the `assetHash` `H`.
```solidity
function _handleSupplies(address pool, address asset, uint256 tokenId, uint256 balance) internal {
  bytes32 _assetHash = assetHash(pool, asset, false);  // H
  uint256 _currentBalance = _balances[tokenId][_assetHash];  // 0

  _updateReward(tokenId, _assetHash);
  _balances[tokenId][_assetHash] = balance; // 100
  _totalSupply[_assetHash] = _totalSupply[_assetHash] - _currentBalance + balance;
}

Those values would be as below:

• Total supply: totalSupply[H] = 200

• Balances: _balances[U1][H] = _balances[U2][H] = 100

  After some time:

• The liquidity index increases to 2.

• A new user, U3, deposits 110 units of A into the pool P.

• U2 makes a small withdrawal of just 1 wei to trigger an update to their balance.

Now, the total supply and user balances for assetHash H become:

• _balances[U1][H] = 100
• _balances[U2][H] = 100 ÷ 1.5 × 2 = 133.3
• _balances[U3][H] = 110
• totalSupply[H] = 343.3

At this point, User U1’s asset balance in the pool P is the largest, being 1 wei more than U2's and 23.3 more than U3's. Yet, U1 receives the smallest rewards because their balance was never updated in the NFTPositionManager. In contrast, User U2 receives more rewards due to the balance update caused by withdrawing just 1 wei.

The issue:

This system is unfair because:

• User U3, who has fewer assets in the pool than U1, is receiving more rewards.
• The rewards distribution favors users who perform frequent updates (like deposits o withdrawals), which is not equitable.

The solution:

Instead of using the asset balance as the rewards basis, we should use the shares in the pool. Here’s how the updated values would look:

• _balances[U1][H] = 66.67
• _balances[U2][H] = 66.67 - 1 wei
• _balances[U3][H] = 110 ÷ 2 = 55
• totalSupply[H] = 188.33

This way, the rewards distribution becomes fair, as it is based on actual contributions to the pool.

Impact

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/positions/NFTPositionManagerSetters.sol#L57-L58

Tool used

Manual Review

Recommendation

function _supply(AssetOperationParams memory params) internal nonReentrant {
  pool.supply(params.asset, address(this), params.amount, params.tokenId, params.data);

-  uint256 balance = pool.getBalance(params.asset, address(this), params.tokenId);
+  uint256 balance = pool.getBalanceRaw(params.asset, address(this), params.tokenId).supplyShares;

  _handleSupplies(address(pool), params.asset, params.tokenId, balance);
}

The same applies to the _borrow, _withdraw, and _repay functions.

[nevillehuang]

request poc

Is there a permisionless update functionality?

[sherlock-admin4]

PoC requested from @etherSky111

Requests remaining: 25

[etherSky111]

Thanks for judging.

There is a clear issue. https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/473 To test this issue properly, we need to resolve the above issue first.

function getSupplyBalance(DataTypes.PositionBalance storage self, uint256 index) public view returns (uint256 supply) {
-  uint256 increase = self.supplyShares.rayMul(index) - self.supplyShares.rayMul(self.lastSupplyLiquidtyIndex);
-  return self.supplyShares + increase;
- 
+  return self.supplyShares.rayMul(index);
}

Below is test code.

function supplyForUser(address user, uint256 supplyAmount, uint256 tokenId, bool mintNewToken) public {
  uint256 mintAmount = supplyAmount;
  DataTypes.ExtraData memory data = DataTypes.ExtraData(bytes(''), bytes(''));
  INFTPositionManager.AssetOperationParams memory params =
    INFTPositionManager.AssetOperationParams(address(tokenA), user, supplyAmount, tokenId, data);

  _mintAndApprove(user, tokenA, mintAmount, address(nftPositionManager));

  vm.startPrank(user);
  if (mintNewToken == true) {
    nftPositionManager.mint(address(pool));
  }
  nftPositionManager.supply(params);
  vm.stopPrank();
}

function borrowForUser(address user, uint256 borrowAmount, uint256 tokenId) public {
  DataTypes.ExtraData memory data = DataTypes.ExtraData(bytes(''), bytes(''));
  INFTPositionManager.AssetOperationParams memory params =
    INFTPositionManager.AssetOperationParams(address(tokenA), user, borrowAmount, tokenId, data);

  vm.startPrank(user);
  nftPositionManager.borrow(params);
  vm.stopPrank();
}

function testRewardDistribution() external {
  DataTypes.ReserveData memory reserveData_0 = pool.getReserveData(address(tokenA));
  console2.log('initial liquidity index                => ', reserveData_0.liquidityIndex);

  address U1 = address(11);
  address U2 = address(12);
  address U3 = address(13);

  /**
    User U1 wants to mint a new NFT (tokenId = 1) and supply 100 ether token
    */
  supplyForUser(U1, 100 ether, 1, true);
  /**
    User U2 wants to mint a new NFT (tokenId = 2) and supply 100 ether token
    */
  supplyForUser(U2, 100 ether, 2, true);

  bytes32 assetHash = nftPositionManager.assetHash(address(pool), address(tokenA), false);

  uint256 balancesOfU1 = nftPositionManager.balanceOfByAssetHash(1, assetHash);
  uint256 balancesOfU2 = nftPositionManager.balanceOfByAssetHash(2, assetHash);
  console2.log('initial balance of U1 for rewards      => ', balancesOfU1);
  console2.log('initial balance of U2 for rewards      => ', balancesOfU2);

  /**
    For testing purposes, Alice mints a new NFT (tokenId = 3), supplies 1000 ether, and borrows 600 Ether. 
    This action increases the pool's liquidity rate to a non-zero value.
    */
  supplyForUser(alice, 1000 ether, 3, true);
  borrowForUser(alice, 600 ether, 3);

  DataTypes.ReserveData memory reserveData_1 = pool.getReserveData(address(tokenA));
  console2.log('current liquidity rate                 => ', reserveData_1.liquidityRate);

  /**
    Skipping 2000 days is done for testing purposes to increase the liquidity index. 
    In a real environment, the liquidity index would increase continuously over time.
    */
  vm.warp(block.timestamp + 2000 days);

  pool.forceUpdateReserve(address(tokenA));
  DataTypes.ReserveData memory reserveData_2 = pool.getReserveData(address(tokenA));
  console2.log('updated liquidity index                => ', reserveData_2.liquidityIndex);

  /**
    User U2 supplies 100 wei (a dust amount) to trigger an update of the balances for rewards.
    */
  supplyForUser(U2, 100, 2, false);

  uint256 balancesOfU1Final = nftPositionManager.balanceOfByAssetHash(1, assetHash);
  uint256 balancesOfU2Final = nftPositionManager.balanceOfByAssetHash(2, assetHash);
  console2.log('final balance of U1 for rewards        => ', balancesOfU1Final);
  console2.log('final balance of U2 for rewards        => ', balancesOfU2Final);

  /**
    User U3 wants to mint a new NFT (tokenId = 4) and supply 110 ether token
    */
  supplyForUser(U3, 110 ether, 4, true);
  uint256 balancesOfU3Final = nftPositionManager.balanceOfByAssetHash(4, assetHash);
  console2.log('final balance of U3 for rewards        => ', balancesOfU3Final);
}

Everything is in below log:

initial liquidity index                =>  1000000000000000000000000000
initial balance of U1 for rewards      =>  100000000000000000000
initial balance of U2 for rewards      =>  100000000000000000000
current liquidity rate                 =>  43490566037735849056603774
updated liquidity index                =>  1238304471439648487981390542
final balance of U1 for rewards        =>  100000000000000000000
final balance of U2 for rewards        =>  123830447143964848898
final balance of U3 for rewards        =>  110000000000000000000

There is no reward system that requires users to continuously update their balances. How can users realistically update their balances every second to receive accurate rewards? Is this practical?

[0xspearmint1]

escalate

This issue does not meet Sherlock's criteria for a medium issue that requires the following:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

For this issue to cause a 0.01% loss there must be an unrealistic increase in the liquidityIndex in an extremely small 14 day period. The POC provided inflates the liquidity index by borrowing a 60% of the funds at a huge interest rate for 5.5 years, this is absolutely not realistic and will never happen.

[sherlock-admin3]

escalate

This issue does not meet Sherlock's criteria for a medium issue that requires the following:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

For this issue to cause a 0.01% loss there must be an unrealistic increase in the liquidityIndex in an extremely small 14 day period. The POC provided inflates the liquidity index by borrowing a 60% of the funds at a huge interest rate for 5.5 years, this is absolutely not realistic and will never happen.

You've created a valid escalation!

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.

[aliX40]

This issue is a high severity bug:

• Tracking shares instead of assets is basically 101 of staking rewards contracts.
• There is a provable and pocable significant loss/theft of yield (more than 1%)
• Rewards Accounting is completly false

[obou07]

escalate per comment

[sherlock-admin3]

escalate per comment

You've created a valid escalation!

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.

[cvetanovv]

I think this issue can be of High severity.

The attack path described in #58 shows very well how a user can deposit 1 USDC every day and earn more rewards than a normal user.

To execute this attack, we have almost no restrictions( except the normal ones, to have a reward and interest rate above zero), and the losses exceed 1%.

Planning to accept @obou07 escalation and make this issue High.

[0xSpearmint]

@cvetanovv This issue has a severe constraint:

1. The other users must not update their position at all for an extended period of time (~4 months to create a 1% difference). This is an external constraint out of the control of the attacker. Furthermore, since a reward EPOCH lasts only 2 weeks it is likely that users will redeem rewards and then compound them back into their position, this totally protects them from the issue.

[samuraii77]

The intended design is for users to NOT update their position, thus it is expected for users to not update their positions for prolonged periods of time. For that reason, the used word "constraint" is not quite correct, it is not a constraint, it is the expected scenario.

[0xSpearmint]

High severity states

Definite loss of funds without (extensive) limitations of external conditions. The loss of the affected party must exceed 1%.

What I described is an external condition (user does not update their position at all, for an extended period of time) that looks extensive to me. All it takes is for a user to supply/withdraw from their position once in a 4 month period to make this issue have very low impact.

[samuraii77]

That is a limitation but not an extensive one as I mentioned in my above comment - users have absolutely no reason to update their positions usually. The only reason people would update their position is due to the issue mentioned in this report which means that only a few users who have a good understanding of Solidity and a rather malicious mindset would update their positions.

Furthermore, it only takes 1 user not updating his position to cause a loss of funds.

[0xSpearmint]

It is ridiculous to say the only reason people will update their position is this issue. Users modify their positions for a multitude of reasons in DEFI (moving to a different pool with more yield, compounding rewards, etc).

[etherSky111]

As a normal DeFi user, will you update your position continuously?

[0xSpearmint]

All the user has to do is update their position once every few months.

[cvetanovv]

@0xSpearmint is right. Most users are short-term investors(rather than long-term) who would update their positions more frequently. The chances of someone updating their position at least once every few months are huge and do not match the High severity rule, whereby there shall be no limitations.

My decision is to reject both escalations and leave this issue Medium severity.

[iamnmt]

@cvetanovv

Most users are short-term investors(rather than long-term) who would update their positions more frequently.

I think it is not fair to make that assumption. It is a subjective assumption. It is equally likely a user is a short-term investor or a long-term investor. The impact for the long-term investor satisfies the high severity requirement.

[0xSpearmint]

ALL investors (short term/ long term) are incentivised to compound rewards back into their positions ASAP to get a greater return.

[etherSky111]

Of course, the final decision is up to @cvetanovv and I don't want to argue. But @0xSpearmint is thinking wrongly.

ALL investors (short term/ long term) are incentivised to compound rewards back into their positions ASAP to get a greater return.

Could you please let me know about any other reward system where stakers should update their positions repeatedly to get a correct rewards? This is obviously a bug and there is no guarantee that all investers should update their positions ASAP to get a greater return.

As a long-term inversters, how do they know whether their rewards are calculated wrongly if they didn't update their positions? Maybe the protocol notify to them?

[0xSpearmint]

I agree this is an issue. But it does not meet sherlock's strict criteria for high severity.

Here is sherlock's criteria for medium:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.

This issue requires a specific external condition, the other users must not update their position at all for an extended period of time (4 months).

[etherSky111]

This is my last comment.

I agree this is an issue. But it does not meet sherlock's strict criteria for high severity.

Here is sherlock's criteria for medium:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.

This issue requires a specific external condition, the other users must not update their position at all for an extended period of time (4 months).

This is not a specific external condition. Imagine there are 100 stakers and are you sure that all these users update their positions in 4 months? If I am a long term staker and I deposited large tokens to get ZERO token rewards, I won't update my positions for a long period as I believe the rewards calculation is correct. Unfortunately, there is an error in the rewards calculation and I will lose funds accidently. But I think this is at most medium issue because this is my mistake to not update my positions accordingly. I should've update my positions every 4 months.

And please stop arguing and let the judge decide.

[cvetanovv]

This is the rule for High severity:

Definite loss of funds without (extensive) limitations of external conditions. The loss of the affected party must exceed 1%.

We only have a 1% loss if someone doesn't update their position for a few months. This is a serious limitation. To be High severity, there should be no limitation as written in the rule.

But it perfectly fits the Medium severity rule:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

My decision is to reject both escalations and leave this issue Medium severity.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 0xspearmint1: rejected
• obou07: rejected