Neo Tokyo contest - carlitox477's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1154-L116 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L120 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1298 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1387-L1393 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1427-L1430 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#L1673

Vulnerability details

Unchecked blocks and rounding errors result in unbounded minting of Bytes 2.0 Tokens, allowing to draining LP Liquidity

Description

The current _stakeLP(uint) function can be summarized in the following pseudocode:

1. Retrieve amount (the amount of LP to stake) from the call stack and transfer this amount to the staking contract.
2. Retrieve the time lock duration and time lock multiplier from the function input.
3. If no multiplier is registered, then no previous stake was done, so we should save the time lock multiplier sent by the parameter. Otherwise, we should verify that the current multiplier saved corresponds to the multiplier we want to use. This means that the time lock for a new LP stake should be the same as the current one.
4. Save or update the msg.sender's LP staking information and update the LP pool information.
5. Emit the stake event.

The problem relays on step 4, which corresponds to next code

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

In particular uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR; can present rounding error if amount < 10^16. This would mean that amount * 100 / 1e18 = 0, then:

1. points = 0
2. stakerLPPosition[msg.sender].points would not be updated
3. pool.totalPoints would not be updated

On the other hand, _withdrawLP() can be summarized in next pseudocode:

1. Get amount to withdraw from call stack
2. Get msg.sender LP position
3. Check that the time lock end time has been reached.
4. Check that the amount in the stake position is enough large to withdraw
5. Transfer LP amount to msg.sender
6. Get LP pool data
7. Update points and amount for LP position

The last step corresponds to next block of code:

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

The line uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR; can be used to the advantage of someone who has made multiple small stakes. For example, if someone has made deposits of 10^16-1 LP tokens, then no points would be added. However, if this same person tries to withdraw their entire position at once, then they can benefit from the underflow and receive more points than they are supposed to.

This can be done by calling withdraw(AssetType,uint) function, when IByteContract(BYTES).getReward(msg.sender); is called, claimReward(address) is called too.

The only way to claim rewards is by calling Bytes.getReward function, when this function calls claimReward(address), it is in getPoolReward where the points corresponding to a position are used to calculate the shares to mint.

Then, it is at the end of the function where this value, gathered with others, is used to calculate how much Bytes are going to be minted

If msg.sender has just LP staked, then getPoolReward(AssetType,address) is called to calculate the amount to mint to withdraw msg.sender and the amount to mint to the dao treasury.

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

If a user manage to get advantage of the underflow that function withdraw offers, then this same user can take advantage of function getPoolReward to mint more Bytes than they are supposed to.

Impact

A user has the ability to take advantage of rounding and underflow in various parts of the code by making multiple small stakes, thereby minting an unbounded amount of BYTES tokens that they can sell on any decentralized exchange (DEX) and drain the liquidity of Bytes 2.0 tokens.

POC

To run this POC run npx hardhat test --grep "POC: Infinite minting due to underflow"

Mitigation steps

To solve this problem the code can be change in order to recalculate the whole points of a position each time a position is modified. This means:

// L1154-1165 in function _stakeLP(uint256)
+   uint256 currentPoints = stakerLPPosition[msg.sender].points;
+   uint256 currentStake = stakerLPPosition[msg.sender].amount;
    unchecked {
-       uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
+   	uint256 newStake = currentStake + amount
+       uint256 newPoints = (amount + currentStake) * 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;
+       stakerLPPosition[msg.sender].points = newPoints;

        // Update the pool point weights for rewards.
-       pool.totalPoints += points;
+       uint256 poolTotalPoint = pool.totalPoints
+       pool.totalPoints = poolTotalPoint - currentPoints + newPoints;
    }

    // function _withdrawLP() L1622-1631
+   uint256 currentStake = lpPosition.amount;
+   uint256 currentPoints = lpPosition.points;
    unchecked {
-       uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
+       uint256 newPoints = (currentStake - amount) * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
+       uint256 pointsDifference = currentPoints - newPoints;

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

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

Doing this should prevent taking advantage from the underflow and rounding mistakes

_assetTransferFrom and _assetTransfer should check if _asset is a contract

Otherwise the call will be successful if it the asset is not a contract

Add pool view function to NeoTokyoStaker

During the audit I was forced to create these method to check the states of the pools. This methods are useful for testing but also for UX, therefore, they should be considered to be added to the staking contract

    struct PoolInfoSummary{
		uint256 totalPoints;
		uint256 daoTax;
		uint256 rewardCount;
		RewardWindow[] rewardWindows;
	}

	struct PoolsInfoSummary{
		PoolInfoSummary s1CitizenPool;
		PoolInfoSummary s2CitizenPool;
		PoolInfoSummary lpPool;
		PoolInfoSummary bytesPool;
	}

    function getRewardWindowArray(PoolData storage pool) internal view returns(RewardWindow[] memory){
		uint256 rewardCount = pool.rewardCount;
		mapping ( uint256 => RewardWindow ) storage rewardMapping = pool.rewardWindows;
		RewardWindow[] memory rewards = new RewardWindow[](rewardCount);
		for(uint256 i; i < rewardCount; i++){
			rewards[i] = rewardMapping[i];
		}
		return rewards;
	}

    function getPoolsInfo() external view returns (PoolsInfoSummary memory){
		PoolData storage s1CitizenPool = _pools[AssetType.S1_CITIZEN];
		PoolData storage s2CitizenPool = _pools[AssetType.S2_CITIZEN];
		PoolData storage lpPool = _pools[AssetType.LP];
		PoolData storage bytesPool = _pools[AssetType.BYTES];

		PoolInfoSummary memory s1CitizenPoolInfo = PoolInfoSummary({
			totalPoints: s1CitizenPool.totalPoints,
			daoTax: s1CitizenPool.daoTax,
			rewardCount: s1CitizenPool.rewardCount,
			rewardWindows: getRewardWindowArray(s1CitizenPool)
		});

		PoolInfoSummary memory s2CitizenPoolInfo = PoolInfoSummary({
			totalPoints: s2CitizenPool.totalPoints,
			daoTax: s2CitizenPool.daoTax,
			rewardCount: s2CitizenPool.rewardCount,
			rewardWindows: getRewardWindowArray(s2CitizenPool)
		});

		PoolInfoSummary memory lpPoolInfo = PoolInfoSummary({
			totalPoints: lpPool.totalPoints,
			daoTax: lpPool.daoTax,
			rewardCount: lpPool.rewardCount,
			rewardWindows: getRewardWindowArray(lpPool)
		});

		PoolInfoSummary memory bytesPoolInfo = PoolInfoSummary({
			totalPoints: bytesPool.totalPoints,
			daoTax: bytesPool.daoTax,
			rewardCount: bytesPool.rewardCount,
			rewardWindows: getRewardWindowArray(bytesPool)
		});
		
		PoolsInfoSummary memory poolsInfo = PoolsInfoSummary({
			s1CitizenPool: s1CitizenPoolInfo,
			s2CitizenPool: s2CitizenPoolInfo,
			lpPool: lpPoolInfo,
			bytesPool: bytesPoolInfo
		});

		return poolsInfo;
	}

DOS to getPoolReward if enough rewards are added to a pool

If enough rewards are set for a pool, block gas would be reached when getPoolReward is called, DOSing claimReward function, forbidding stakers from claiming their corresponding rewards.

Currently, pool rewards can be defined by configurePools function by an address with corresponding permission. It would be advisable to add a max number of rewards check to add to a pool, in order to avoid DOS getPoolReward function.

NeoTokyoStaker assumes LP decimals are 18

Even though nowadays every LP contract has it decimals set to 18, it would be advisable to allow setting this value, given that LP can be set again through configureLP function, in case someday the protocol decide to use an unconventional type of LP. This would also means adding a new variable called lpDecimals to change whenever LP changes.

withdraw: Should check that uint8(_assetType) > 3 instead of uint8(_assetType) > 4

Given there is no case for 4 defined below, 4 should also be taken into account to revert the function. This means:

    // NeoTokyoStaker.withdraw
-   if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
+   if (uint8(_assetType) == 2 || uint8(_assetType) > 3) {
        revert InvalidAssetType(uint256(_assetType));
    }

VAULT_CAP, LP and NO_VAULT_CAP should follow conventional naming

Variable which are not constants/immutables should be named following lower camel case naming convention

PoolData struct can package daoTax and rewardCount

Given that a daoTax of 100% means that daoTax should be equal to 100_00; and that the chances of a pool to have more than 2^128 rewards is highly unlikely, We can use two uint128 variables in order to save one slot. This would mean:

    struct PoolData {
		uint256 totalPoints;
-   	uint256 daoTax;
-		uint256 rewardCount;
+       uint128daoTax;
+		uint128 rewardCount;
		mapping ( uint256 => RewardWindow ) rewardWindows; // Hold emition start date and its rate
	}

StakedS1Citizen struct can package timelockEndTime and hasVault

Given that timelockEndTime is meant to be a timestamp, a slot can be saved by modifying struct element order and type:

	struct StakedS1Citizen {
		uint256 stakedBytes;
-		uint256 timelockEndTime;
		uint256 points;
		uint256 stakedVaultId;
		bool hasVault;
+       uint40 timelockEndTime;
	}

LPPosition struct can package timelockEndTime and multiplier

Given that timelockEndTime is meant to be a timestamp, and that multiplier is meant to be lower than 2^128.a slot can be saved by modifying struct element order and type:

	struct LPPosition {
		uint256 amount;
-		uint256 timelockEndTime;
		uint256 points;
		uint256 multiplier;
+       uint128 multiplier;
+       uint40 timelockEndTime;
	}

StakedS1CitizenOutput struct can package timelockEndTime and hasVault

Given that timelockEndTime is meant to be a timestamp, a slot can be saved by modifying struct element order and type:

    struct StakedS1CitizenOutput {
		uint256 citizenId;
		uint256 stakedBytes;
-		uint256 timelockEndTime;
		uint256 points;
		uint256 stakedVaultId;
		bool hasVault;
+       uint40 timelockEndTime;
	}

getStakerPositions(address) can use storage pointer to save gas inside for loop

The use of storage pointer instead of direct access can save gas in for loops.

	function getStakerPositions (
		address _staker
	) external view returns (StakerPosition memory) {

		// Compile the S1 Citizen details.
+		uint256[] storage stakerS1Position = _stakerS1Position[_staker];
+		uint256 stakerS1PositionLen = stakerS1Position.length
		StakedS1CitizenOutput[] memory stakedS1Details =
			new StakedS1CitizenOutput[](_stakerS1Position[_staker].length);
-		for (uint256 i; i < _stakerS1Position[_staker].length; ) {
+		for (uint256 i; i < stakerS1PositionLen; ) {
-			uint256 citizenId = _stakerS1Position[_staker][i];
+			uint256 citizenId = stakerS1Position[i];
			StakedS1Citizen memory citizenDetails = stakedS1[_staker][citizenId];
			stakedS1Details[i] = StakedS1CitizenOutput({
				citizenId: citizenId,
				stakedBytes: citizenDetails.stakedBytes,
				timelockEndTime: citizenDetails.timelockEndTime,
				points: citizenDetails.points,
				hasVault: citizenDetails.hasVault,
				stakedVaultId: citizenDetails.stakedVaultId
			});
			unchecked { i++; }
		}

		// Compile the S2 Citizen details.
+		uint256[] storage stakerS2Position = _stakerS2Position[_staker]
+		uint256 stakerS2PositionLen = stakerS2Position.length
		StakedS2CitizenOutput[] memory stakedS2Details =
-			new StakedS2CitizenOutput[](_stakerS2Position[_staker].length);
+			new StakedS2CitizenOutput[](_stakerS2Position[_staker].length);
+		mapping(uint256 => StakedS2Citizen) storage _stakedS2 = stakedS2[_staker];
-		for (uint256 i; i < _stakerS2Position[_staker].length; ) {
+		for (uint256 i; i < stakerS2PositionLen; ) {
-			uint256 citizenId = _stakerS2Position[_staker][i];
+			uint256 citizenId = stakerS2Position[i];
-			StakedS2Citizen memory citizenDetails = stakedS2[_staker][citizenId];
+			StakedS2Citizen memory citizenDetails = _stakedS2[citizenId];
			stakedS2Details[i] = StakedS2CitizenOutput({
				citizenId: citizenId,
				stakedBytes: citizenDetails.stakedBytes,
				timelockEndTime: citizenDetails.timelockEndTime,
				points: citizenDetails.points
			});
			unchecked { i++; }
		}

		// Return the final output position struct.
		return StakerPosition({
			stakedS1Citizens: stakedS1Details,
			stakedS2Citizens: stakedS2Details,
			stakedLPPosition: stakerLPPosition[_staker]
		});
	}

getPoolReward(AssetType,address) can use storage pointer to save gas inside for loop

The use of storage pointer instead of direct access can save gas in for loops.

	if (_assetType == AssetType.S1_CITIZEN) {
+		uint256[] storage stakerS1Positions = _stakerS1Position[_recipient];
+		mapping(uint256 => StakedS1Citizen) storage _stakedS1 = stakedS1[_recipient];
+		uint256 stakerS1PositionsLen = stakerS1Positions.length
-		for (uint256 i; i < _stakerS1Position[_recipient].length; ) {
+		for (uint256 i; i < stakerS1PositionsLen; ) {
-			uint256 citizenId = _stakerS1Position[_recipient][i];
+			uint256 citizenId = stakerS1Positions[i];
-			StakedS1Citizen memory s1Citizen = stakedS1[_recipient][citizenId];
+			StakedS1Citizen memory s1Citizen = _stakedS1[citizenId];
			unchecked {
				points += s1Citizen.points;
				i++;
			}
		}
	} else if (_assetType == AssetType.S2_CITIZEN) {
+		uint256[] storage stakerS2Positions = _stakerS2Position[_recipient];
+		mapping(uint256 => StakedS2Citizen) storage _stakedS2 = stakedS2[_recipient];
+		uint256 stakerS2PositionsLen = stakerS2Positions.length
-		for (uint256 i; i < _stakerS2Position[_recipient].length; ) {
+		for (uint256 i; i < stakerS2PositionsLen; ) {
-			uint256 citizenId = _stakerS2Position[_recipient][i];
+			uint256 citizenId = stakerS2Positions[i];
-			StakedS2Citizen memory s2Citizen = stakedS2[_recipient][citizenId];
+			StakedS2Citizen memory s2Citizen = _stakedS2[citizenId];
			unchecked {
				points += s2Citizen.points;
				i++;
			}
		}

PoolConfigurationInput struct can package daoTax and rewardCount

Given that a daoTax of 100% means that daoTax should be equal to 100_00; and that AssetType is uint8, we can use a uint128 variable in order to save one slot. This would mean:

    struct PoolConfigurationInput {
		AssetType assetType;
-   	uint256 daoTax;
+       uint128 daoTax;
		RewardWindow[] rewardWindows;
	}

NeoTokyoStaker.configurePools use storage pointer in loop to avoid redundant access

_pools[_inputs[i].assetType] can be save in a storage variable to avoid looking for its reference multiple times in the loop. This means

    for (uint256 i; i < _inputs.length; ) {
        uint256 poolRewardWindowCount = _inputs[i].rewardWindows.length;
+       PoolData storage _pool = _pools[_inputs[i].assetType]
-       _pools[_inputs[i].assetType].rewardCount = poolRewardWindowCount;
+       _pool.rewardCount = poolRewardWindowCount;
-       _pools[_inputs[i].assetType].daoTax = _inputs[i].daoTax;
+       _pool.daoTax = _inputs[i].daoTax;

        // Set the pool reward window details by populating the mapping.
        uint256 lastTime;
        for (uint256 j; j < poolRewardWindowCount; ) {
-           _pools[_inputs[i].assetType].rewardWindows[j] =
+           _pool.rewardWindows[j] =
                _inputs[i].rewardWindows[j];

            // Revert if an invalid pool configuration is supplied.
            if (j != 0 && _inputs[i].rewardWindows[j].startTime <= lastTime) {
                revert RewardWindowTimesMustIncrease();
            }
            lastTime = _inputs[i].rewardWindows[j].startTime;
            unchecked { j++; }
        }
        unchecked { ++i; }
    }