veRWA - auditsea's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/498a3004d577c8c5d0c71bff99ea3a7907b5ec23/src/GaugeController.sol#L211-L278

Vulnerability details

Impact

The issue is applied differently based on how change_gauge_weight works.

1. When changing gauge weight is essential for every enabled gauge before any vote happens

An attacker can front-run change_gauge_weight transaction to manipulate slope which can result in reverts in the future in getting the gauge weight

2. When changing gauge weight is optional for gauges

Vote does not have effect before change_gauge_weight is called once.

Proof of Concept

  function _get_weight(address _gauge_addr) private returns (uint256) {
      uint256 t = time_weight[_gauge_addr];
      if (t > 0) {
        // time_weight[_gauge_addr] is updated here
        ...
      } else {
          return 0;
      }
  }

In _get_weight function, time_weight is updated to next_time when original value is set. The time_weight for a gauge is initialized only through change_gauge_weight.

So when vote_for_gauge_weights is called before time_weight is initialized, _get_weight always return zero and time_weight also remains zero. Thus, updating points_weight has no effect but changes_weight has effect.

  function _get_weight(address _gauge_addr) private returns (uint256) {
      uint256 t = time_weight[_gauge_addr];
      if (t > 0) {
        ...
          if (pt.bias > d_bias) {
              pt.bias -= d_bias;
              uint256 d_slope = changes_weight[_gauge_addr][t];
              pt.slope -= d_slope; // <-- where issue happens
          } else {
              pt.bias = 0;
              pt.slope = 0;
          }
        ...
      } else {
          return 0;
      }
  }

Assuming there are some vote transactions that are front-run before change_gauge_weight is called, at some point in the future, _get_weight can be reverted on the line mentioned above. Because, points_weight is not changed so pt.bias > d_bias will be true, but inside it, pt.slope will be smaller than d_slope since there had been changes made to changes_weight.

This will cause revert, once this is reverted, the GaugeController will not work anymore.

Used Tools

Manual Review

Recommended Mitigation Steps

There can be more than one ways to resolve this issue as follows:

1. Update gauge weight to zero when add_gauge is called. (as in remove_gauge)

  function add_gauge(address _gauge) external onlyGovernance { // @audit-ok
      require(!isValidGauge[_gauge], "Gauge already exists");
      _change_gauge_weight(_gauge, 0);
      isValidGauge[_gauge] = true;
      emit NewGauge(_gauge);
  }

2. In vote_for_gauge_weights, set time_weight to next_time

  points_weight[_gauge_addr][next_time].bias = Math.max(old_weight_bias + new_bias, old_bias) - old_bias;
  points_sum[next_time].bias = Math.max(old_sum_bias + new_bias, old_bias) - old_bias;
  time_weight[_gauge_addr] = next_time; // <-- add this line

3. Handle slope error just in case

if (pt.bias > d_bias) {
    pt.bias -= d_bias;
    uint256 d_slope = changes_weight[_gauge_addr][t];

    // update like this
    if (pt.slope > d_slope) {
      pt.slope -= d_slope;
    } else {
      pt.slope = 0;
    }
} else {
    pt.bias = 0;
    pt.slope = 0;
}

Appling one of these would resolve the issue, but appling all of them would make it perfect!

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L166-L177

Vulnerability details

Impact

LendingLedger -> claim function iterates through weeks and claim rewards per week for the user. However it will revert when market balance is zero for a specific epoch, which will not let users claim rewards.

Proof of Concept

Imagine of the short period when USDC price depegged. In that case, users will try to withdraw their money from markets, which might cause zero balance for the market. When USDC price is balanced again, users would try to deposit again to the market. So there can be specific epoches where market balance is zero.

for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
    uint256 userBalance = lendingMarketBalances[_market][lender][i];
    uint256 marketBalance = lendingMarketTotalBalance[_market][i];
    RewardInformation memory ri = rewardInformation[i];
    require(ri.set, "Reward not set yet"); // Can only claim for epochs where rewards are set, even if it is set to 0
    uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
    cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
}

However in the codebase, it's not checking if marketBalance is zero, which means that zero maket balance at a specific epoch will cause revert claiming rewards.

Used Tools

Manual Review

Recommended Mitigation Steps

for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
    uint256 userBalance = lendingMarketBalances[_market][lender][i];
    uint256 marketBalance = lendingMarketTotalBalance[_market][i];

    if (marketBalance == 0) continue;

    RewardInformation memory ri = rewardInformation[i];
    require(ri.set, "Reward not set yet"); // Can only claim for epochs where rewards are set, even if it is set to 0
    uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
    cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
}

Continue loop when market balance is zero will resolve it.

Assessed type

Math