Neo Tokyo contest - nobody2018's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1621-L1636

Vulnerability details

Impact

After user withdrawn all lptokens, he/she cannot continue to receive rewards. However, due to the problem of integer division, attacker can carefully construct parameters to withdraw all deposited lptoken and continue to receive rewards. This causes other users' reward losses.

Proof of Concept

Let's assume that lpPosition.multiplier is 2, and attacker stakes 100e18 lptoken. According to the formula uint256 points=amount * 100/1e18 * lpPosition.multiplier/_DIVISOR, the attacker's points is 200. As time goes by, the attacker can withdraw all deposited lptoken. The attack pseudocode is as follows:

contract Fun {
    ...
    
    function start() {
        uint256 allLp = 200e18;
        uint256 count = allLp / 99e16;
        uint256 remain = allLp - 99e16 * count;
        for(uint256 i; i < count; ++i) {
            NeoTokyoStaker.withdraw(AssetType.LP, 99e16);
        }
        if (remain > 0) {
            NeoTokyoStaker.withdraw(AssetType.LP, remain);
        }
    }
    
    ...
}

After the code is executed, attacker gets all deposited lptoken, but points is still 200 that is the most important parameter to get rewards. Let's see why points has not been reduced. Code snippet for _withdrawLP

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

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

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

So, attacker only withdraws 99e16 lptoken every time while decreased points is 0. At present, the cost of this attack on the main network will very high due to the expensive gas fee. It obviously doesn't work. But we should consider two situations:

• With the upgrading of the main network, the cost of gas has decreased significantly.
• The project will be deployed in other chains and layer2 where the cost of gas are very low.

Tools Used

Manual Review

Recommended Mitigation Steps

We should change 100 to a large enough number. e12 is a magic number which comes from pancake.

--- a/contracts/staking/NeoTokyoStaker.sol
+++ b/contracts/staking/NeoTokyoStaker.sol
@@ -1620,7 +1620,7 @@ contract NeoTokyoStaker is PermitControl, ReentrancyGuard {
                // Update caller staking information and asset data.
                PoolData storage pool = _pools[AssetType.LP];
                unchecked {
-                       uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
+                       uint256 points = amount * 1e12 / 1e18 * lpPosition.multiplier / 1e12;