Users staking BYTES will accrue 100 times more points than expected because of a miscalculation in `_stakeBytes`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L203 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L10617-L1080 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1085-L1101

Vulnerability details

Impact

A magnitude of 100 times will completely break the balance on the rewards system calculation, as staking S1 Citizens, S2 Citizens and LP tokens will accrue 100 times less in comparison, and ultimately those users will only be able to claim a fraction of the rewards compared to the ones staking BYTES.

This will break the rewards system, and greatly discourage users from staking LP tokens, or from staking their Citizens without staking BYTES.

Proof of Concept

The _stakeBytes() function for staking BYTES with S1 and S2 Citizens has an error when calculating the bonusPoints.

As per the documentation: "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."

This means 200 BYTES per point.

The code defines it as: bonusPoints = (amount * 100 / _BYTES_PER_POINT)

Which translates to: bonusPoints = amount * 100 / (200 * 1e18)

This results in the miscalculation that it only needs: 2 BYTES per point, which is 100 times lower than expected.

This may be due to some confusion between "base points" and "basis points". 1 base point = 100 basis points.

bonusPoints are added to both citizenStatus.points and pool.totalPoints. So they must be expressed in base points.

Examples:

    // For 200 BYTES `amount == 200 * 1e18`
    // Expected: 1 point
    // Result: 100 points
    bonusPoints = (amount * 100 / _BYTES_PER_POINT);
    bonusPoints = amount * 100 / (200 * 1e18);
    bonusPoints = (200 * 1e18) * 100 / (200 * 1e18);
    bonusPoints = 100;

    // For 2 BYTES: `amount == 2 * 1e18`
    // Expected: 0 points
    // Result: 1 point
    bonusPoints = (amount * 100 / _BYTES_PER_POINT);
    bonusPoints = amount * 100 / (200 * 1e18);
    bonusPoints = (2 * 1e18) * 100 / (200 * 1e18);
    bonusPoints = 1;

References:

The definition of _BYTES_PER_POINT:

// File: NeoTokyoStaker.sol

202:    /// The number of BYTES needed to get one point in BYTES staking calculations.
203:    uint256 constant private _BYTES_PER_POINT = 200 * 1e18;

Link to Code

bonusPoints are miscalculated in _stakeBytes() and added to the user and the S1 and S2 pool:

// File: NeoTokyoStaker.sol
// Function: _stakeBytes()
// S1 Citizen

L1061:                  StakedS1Citizen storage citizenStatus = stakedS1[msg.sender][citizenId];

L1075:                              PoolData storage pool = _pools[AssetType.S1_CITIZEN];

L1077:              uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT); // @audit wrong calculation
L1078:              citizenStatus.stakedBytes += amount;
L1079:              citizenStatus.points += bonusPoints; // @audit accrued more points than expected
L1080:              pool.totalPoints += bonusPoints; // @audit imbalance in the pool total points

Link to Code

// File: NeoTokyoStaker.sol
// Function: _stakeBytes()
// S2 Citizen

L1085:                      StakedS2Citizen storage citizenStatus = stakedS2[msg.sender][citizenId];

L1096:                              PoolData storage pool = _pools[AssetType.S2_CITIZEN];

L1098:              uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT); // @audit wrong calculation
L1099:              citizenStatus.stakedBytes += amount;
L1100:              citizenStatus.points += bonusPoints; // @audit accrued more points than expected
L1101:              pool.totalPoints += bonusPoints; // @audit imbalance in the pool total points

Link to Code

Tools Used

Manual Review

Recommended Mitigation Steps

Fix the calculation by removing the * 100:

// File: NeoTokyoStaker.sol
// Function: _stakeBytes()

-               uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
+               uint256 bonusPoints = amount / _BYTES_PER_POINT;

Check that the calculation for LP tokens is done correctly:

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

Link to code

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

Link to code

Fix the corresponding tests, and replace with exact calculations, as the use broad ranges make it difficult to spot these issues:

// Confirm Bob and the DAO received their proper share.
let bobBalance = await NTBytes2_0.balanceOf(bob.address);
bobBalance.sub(bobBalanceInitial).should.be.closeTo(
  ethers.BigNumber.from("8919540229885057471"), // @audit broad range
  ethers.BigNumber.from("1000000000") // @audit broad range
);

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as primary issue

[TimTinkers]

I don't think this is valid.

bonusPoints are added to both citizenStatus.points and pool.totalPoints. So they must be expressed in base points. bonusPoints = 100 = "one point", these numbers are all denominated in basis points.

bonusPoints = amount * 100 / (200 * 1e18) -> amount = 200 BYTES = 200 * 1e18 -> bonusPoints = 100 = one logical point. Unless I got very unlucky with the selection of values for my test cases, the math in those behaves the way I want it to...

[c4-sponsor]

TimTinkers marked the issue as sponsor disputed

[hansfriese]

When we check other functions like _stakeLP(), points are calculated like this. uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR; It means the protocol considers 100 points as one logical point originally. Downgrade to QA due to inaccurate documentation.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-b

[c4-sponsor]

TimTinkers marked the issue as sponsor confirmed