Neo Tokyo contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In NeoTokyoStaker, staker is a competitive system where stakers compete for a fixed emission rate in each of the S1 Citizen, S2 Citizen, and LP token staking pools. For each staking pool, there are some reward windows. Each reward window has different emission rates configurable by admin. So when calculating the final reward amount for a user, it does a for loop through every reward window and sum them up.

However, it used the final points and totalPoints when staker claimed reward for every previous reward window, which is not correct since totalPoints is different every time other stakers deposit/withdraw.

The result is the reward is incorrectly calculated, it can be higher or lower than expected, and both cases are high risk issues.

Proof of Concept

Consider a scenario:

1. Citizen S2 pools have 2 reward windows. The first starts at t1 = 100 and rate1 = 200, the second starts at t2 = 200 and rate2 = 300

2. Alice and Bob both stakes at t0 = 100, Citizen S2 tokens have the same weight (100) so Alice and Bob should receive equal amounts of rewards at any moment.

3. Alice withdraw first at t3 = 300, her share is equal to 100 and totalPoints = 200, she will receive

totalReward = ((t2 - t1) * rate1 + (t3 - t2) * rate2)
  = 100 * 200 + 100 * 300 = 50000
share = points * totalReward / totalPoints 
  = 100 * 50000 / 200 = 25000
totalPoints = 100

4. Bob withdraw at the same time, but his TX is executed after Alice, he will receive

totalReward = ((t2 - t1) * rate1 + (t3 - t2) * rate2)
  = 100 * 200 + 100 * 300 = 50000
share = points * totalReward / totalPoints 
  = 100 * 50000 / 100 = 50000

As we can see, Alice and Bob both deposit and withdraw at the same time, but Alice only gets 25000 while Bob gets 50000 token reward. Totally, they receive 75000 while totalReward is only 50000, which is more than expected amount of BYTES should be minted.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the formula to calculate rewards for users. For example, in Master Chef, we need to calculate the accumulated reward rate per share of the pool every time a user interacts with the pool and only need to update the position of each user when they interact.

Lines of code

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

Vulnerability details

Impact

In NeoTokyoStaker, BYTES token can be staked into a Citizen. First, the Citizen must be staked, it will be locked for a timelock duration in Staking contract. Staker want to stake BYTES can specify this Citizen ID and stake into it.

However, when users stake into a Citizen, it did not extend the timelock. As the result, attacker can abuse this to manipulate totalPoints, making other stakers receive less rewards.

Proof of Concept

Attacker will stake any Citizen and wait after timelockEndTime to execute the attack. Now consider the scenario when Alice (a normal user) claims her reward

1. Attacker add huge amount of BYTES to his position, effectively increase totalPoints to large value
2. Alice TX is executed. Since totalPoints is manipulate to be larger, Alice's share gets smaller and Alice receives less reward
3. Attacker remove BYTES that he staked. Note that becasue, timelock duration is not extended when attacker stake BYTES, he can remove it in the same block. So the cost for the attack is just gas cost. The amount of BYTES could be his own funds or flash loan since all the attack is happen in 1 block.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider extending timelock duration when stakers stake BYTES into Citizen.

Lines of code

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

Vulnerability details

Impact

In function _withdrawLP(), it calculates the amount of points from the amount input parameter.

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

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

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

However, because of rounding down in calculation, the attacker can withdraw all amount without removing any points. As a result, an attacker's LP position can have points > 0 even though amount = 0, which means attackers still receive rewards without depositing anything.

Proof of Concept

Consider the scenario

1. Alice (attacker) deposits 1e18 LP token to NeoTokyoStaker. Let's just assume multiplier = 10000. Alice will receive the amount of

points = amount * 100 / 1e18 * multiplier / _DIVISOR;
  = 1e18 * 100 / 1e18 * 10000 / 100
  = 10000

2. Alice withdraws 1e16 - 1 wei LP token. The amount of points will be deducted is

points = amount * 100 / 1e18 * multiplier / _DIVISOR;
  = (1e16 - 1) * 100 / 1e18 * 10000 / 100
  = 0

3. As we can see, Alice successfully withdraws 1e16 - 1 wei LP token without losing any points. Alice can repeat step 2 as many times as she wants and then create a position with large points without any LP tokens.
4. In this example, she can repeat 100 times (can be done by deploying a contract to do the for loop) to withdraw all and then repeat from step 1. For each time, she will get 10000 points.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding PRECISION (e.g: 1e18) when calculating points from amount in LP pool. Also consider doing all multiplication before division to avoid precision loss.

Summary

1. Unnecessary check for AssetType

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

Detail

ENUM in Solidity will revert automatically if it is assigned a value that is not predefined. So the check for AssetType in stake() and withdraw() functions are unnecessary.

if (uint8(_assetType) > 4) { // @audit unnecessary check
    revert InvalidAssetType(uint256(_assetType));
}

Recommendation

Remove the AssetType checks.

2. AssetType has value from 0-3, check for uint8(_assetType) > 4 still allow _assetType = 4

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

Detail

AssetType ENUM only has 4 predefined values, so its value is from 0-3. However the check in function stake() and withdraw() are still allow _assetType = 4.

if (uint8(_assetType) > 4) { // @audit value 4 is still allowed
    revert InvalidAssetType(uint256(_assetType));
}

Recommendation

Consider updating the check or removing it since it is unnecessary anyway.

3. Division before multiplication

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

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

Detail

Performing multiplication before division is generally better to avoid loss of precision because Solidity integer division might truncate. There are multiple places in the codebase this issue exists. For example

unchecked {
    // @audit division before multiplication
    uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
    uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
    share /= _PRECISION;
    daoShare /= _PRECISION;
    return ((share - daoShare), daoShare);
}

Recommendation

Consider doing all multiplication before division to avoid truncation.

4. Formula to calculate point in LP pool can be simplified

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

Detail

Formula to calculate points in functions stake() and withdraw() can be simplified since _DIVISOR = 100.

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

Recommendation

So the formula can be simplified to

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