Neo Tokyo contest - auditor0517's results

Lines of code

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

Vulnerability details

Impact

It calculates the staking reward wrongly so users will receive wrong amount of reward.

Proof of Concept

It calculates the staking reward like MasterChef of SusiSwap but it's wrongly implemented.

Current one calculates the reward with the totalPoints of the pool and it's wrong.

• User A staked 100 points at Time 0. totalPoints of the pool is 100 and reward rate is 1 now.
• If he claims the reward at Time 100, his reward will be rewardRate * timeSpan * userPoints / totalPoints = 1 * 100 * 100 / 100 = 100
• But he didn't claim at that time. Instead, User B staked another 100 points at Time 100. totalPoints will be 200 now.
• If User A claims the reward at Time 101, it will be rewardRate * timeSpan * userPoints / totalPoints = 1 * 101 * 100 / 200 = 50.5.
• His reward should be 100 at least but it's decreased because totalPoints was increased by User B before claiming the reward.

So the reward amount will be calculated wrongly according to the totalPoints.

Tools Used

Manual Review

Recommended Mitigation Steps

We should create and track accumulatedRewardPerPoint like MasterChef of SushiSwap and calculate each user's reward accordingly.

Lines of code

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

Vulnerability details

Impact

Users can manipulate their points and steal most of the staking reward.

Proof of Concept

When users stake and withdraw LP, it calculates the points like this.

File: NeoTokyoStaker._stakeLP()
1155: uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

File: NeoTokyoStaker._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;
1628: 
1629:           // Update the pool point weights for rewards.
1630:           pool.totalPoints -= points;
1631:       }

There is a rounding during the point calculation and an attacker can inflate her points easily by staking 2 times(so rounding 2 times) and withdrawing at a time.

• Alice stakes 1e16 + 1 of LP when timelockMultiplier = 100. So her points will be 1.
• Alice stakes 1e16 - 1 of LP again. Her points will be 1 anyway due to the rounding in _stakeLP().
• Alice withdraws 2e16 of LP at a time and the points at L1623 will be 2 and lpPosition.points = 1 - 2 = -1 so her points will be type(uint256).max because it's inside the unchecked block.
• So she managed to inflate her points and the reward system will be broken.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check one condition during the subtraction.

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

    // max cap
    if(points > lpPosition.points) {
        points = lpPosition.points;
    }

    // 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/c49281f27156c9db7f278a60a85885f8c731dc78/contracts/staking/NeoTokyoStaker.sol#L1627

Vulnerability details

Impact

Dust points would remain in the totalPoints after withdrawing all LP

Proof of Concept

When users stake and withdraw LP, it calculates the points like this.

File: NeoTokyoStaker._stakeLP()
1155: uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

File: NeoTokyoStaker._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;
1628: 
1629:           // Update the pool point weights for rewards.
1630:           pool.totalPoints -= points;
1631:       }

Due to the rounding, some points would remain after the withdrawal.

• Alice staked 1e16 of LP and got 1 point when timelockMultiplier = 100.
• After that, she withdrew 5e15 twice and her points will be 1 due to the rounding. Also, totalPoints wasn't decreased.
• So she can claim a reward after withdrawing all LP and other users will receive less amount of reward.

It's a minor amount but it should be fixed for users' preferences.

Tools Used

Manual Review

Recommended Mitigation Steps

When we compare the current staking logic and ERC4626 vault, we may say LP is asset, and points is shares.

Then _withdrawLP() should calculate the points like ERC4626.previewWithdraw().

So we should round up during the points calculation and apply the max cap according to my other submission - Users can inflate their points during LP staking.