Lines of code

Vulnerability details

Impact

Due to unchecked math in the _withdrawLP() function, a user can trigger an underflow in their points and infinitely increase their rewards.

The problem exists in several places.

Problem 1. The configureTimelockOptions() function allows setting timelockMultiplier to zero.

There are two points to note:

1. This is a realistic scenario, as an admin may want to discourage users from extending their positions by setting a zero timelockMultiplier. Thus, passing zero timelockMultiplier maybe considered as a valid behaviour (not a misconfiguration), especially since it is not forbidden in the configureTimelockOptions() function.

2. The timelockMultiplier is passed to the contract in a complex way, packed together with another variable (timelockPeriod) into a single uint256, thus increasing the possibility of mistakes when passing new data.

Taking these two points into account, as well as the fact that the key problem lies in the unchecked math in the _withdrawLP() function, this finding falls within the scope.

Problem 2. The _stakeLP() function contains code that changes the value of multiplier in the user’s position:

if (stakerLPPosition[msg.sender].multiplier == 0) {
  stakerLPPosition[msg.sender].multiplier = timelockMultiplier;

<https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1145&gt;

Thus, if the user created a position with a zero multiplier, they can then call _stakeLP() a second time with a different timelockMultiplier and overwrite their multiplier to a new, positive value.

Problem 3. The _withdrawLP() function contains unchecked math that does not check for underflow when calculating points:

unchecked {
  uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
  
  // Update the caller's LP token stake.
  lpPosition.amount -= amount;
  lpPosition.points -= points;

  // Update the pool point weights for rewards.
  pool.totalPoints -= points;
}

<https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1627&gt;

As a result, a hacker can set points to be greater than what they had in lpPosition.points, but less than pool.totalPoints.

This will cause the value of pool.totalPoints to change only slightly, but lpPosition.points will become a huge value (close to 2 ** 256). The hacker will then be able to claim this huge reward.

Proof of Concept

The PoC is written as a hardhat test and can be downloaded via secret gist:
<https://gist.github.com/knek-little-projects/7dbb2a5dc8315c4c5c3fe114b10b11a1&gt;

Put the test in test/PointsUnderflow.test.js file and run with npx hardhat test test/PointsUnderflow.test.js

The test consists of the following steps:

1. Alice stakes 100 LP with a positive multiplier
2. Bob stakes 2 LP with zero multiplier
3. Bob stakes 0 LP with a positive multiplier
4. Bob withdraws 1 LP. The underflow occurs and Bob gets an immense amount of points.
5. Bob checks his balance and compares it to Alice balance.

The output of the test shows that a normal user (Alice) stakes 100 LP and gets 10000 points with a small amount of reward, while the hacker (Bob) stakes only 2 LP and gets 3037375438411862678027 points and an immense amount of reward:

ALICE STAKES 100 LP
BOB STAKES 2 LP
BOB STAKES 0 LP
SKIP TIME
BOB HAS POINTS 0
BOB HAS REWARD 0
BOB WITHDRAWS 1 LP
SKIP TIME
ALICE HAS POINTS 10000
ALICE HAS REWARD 3037375438411862678027
BOB   HAS POINTS 115792089237316195423570985008687907853269984665640564039457584007913129639836
BOB   HAS REWARD 31570186802875909244984703739959627676612910076697005458645310405

Tools Used

The power of mind

Recommended Mitigation Steps

To prevent the issues described, it is recommended to avoid using unchecked math excessively and to forbid a zero multiplier.

---

The text was updated successfully, but these errors were encountered:

All reactions