Neo Tokyo contest - ulqiorra's results

Lines of code

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

Vulnerability details

Impact

Due to unchecked math in the _withdrawLP() function, a user can trigger an underflow in their points and infinitely increase their rewards.

The problem exists in several places.

Problem 1. The configureTimelockOptions() function allows setting timelockMultiplier to zero.

There are two points to note:

1. This is a realistic scenario, as an admin may want to discourage users from extending their positions by setting a zero timelockMultiplier. Thus, passing zero timelockMultiplier maybe considered as a valid behaviour (not a misconfiguration), especially since it is not forbidden in the configureTimelockOptions() function.

2. The timelockMultiplier is passed to the contract in a complex way, packed together with another variable (timelockPeriod) into a single uint256, thus increasing the possibility of mistakes when passing new data.

Taking these two points into account, as well as the fact that the key problem lies in the unchecked math in the _withdrawLP() function, this finding falls within the scope.

Problem 2. The _stakeLP() function contains code that changes the value of multiplier in the user's position:

if (stakerLPPosition[msg.sender].multiplier == 0) {
  stakerLPPosition[msg.sender].multiplier = timelockMultiplier;

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

Thus, if the user created a position with a zero multiplier, they can then call _stakeLP() a second time with a different timelockMultiplier and overwrite their multiplier to a new, positive value.

Problem 3. The _withdrawLP() function contains unchecked math that does not check for underflow when calculating points:

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

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

As a result, a hacker can set points to be greater than what they had in lpPosition.points, but less than pool.totalPoints.

This will cause the value of pool.totalPoints to change only slightly, but lpPosition.points will become a huge value (close to 2 ** 256). The hacker will then be able to claim this huge reward.

Proof of Concept

The PoC is written as a hardhat test and can be downloaded via secret gist: https://gist.github.com/knek-little-projects/7dbb2a5dc8315c4c5c3fe114b10b11a1

Put the test in test/PointsUnderflow.test.js file and run with npx hardhat test test/PointsUnderflow.test.js

The test consists of the following steps:

1. Alice stakes 100 LP with a positive multiplier
2. Bob stakes 2 LP with zero multiplier
3. Bob stakes 0 LP with a positive multiplier
4. Bob withdraws 1 LP. The underflow occurs and Bob gets an immense amount of points.
5. Bob checks his balance and compares it to Alice balance.

The output of the test shows that a normal user (Alice) stakes 100 LP and gets 10000 points with a small amount of reward, while the hacker (Bob) stakes only 2 LP and gets 3037375438411862678027 points and an immense amount of reward:

ALICE STAKES 100 LP
BOB STAKES 2 LP
BOB STAKES 0 LP
SKIP TIME
BOB HAS POINTS 0
BOB HAS REWARD 0
BOB WITHDRAWS 1 LP
SKIP TIME
ALICE HAS POINTS 10000
ALICE HAS REWARD 3037375438411862678027
BOB   HAS POINTS 115792089237316195423570985008687907853269984665640564039457584007913129639836
BOB   HAS REWARD 31570186802875909244984703739959627676612910076697005458645310405

Tools Used

The power of mind

Recommended Mitigation Steps

To prevent the issues described, it is recommended to avoid using unchecked math excessively and to forbid a zero multiplier.

LOW 1

NeoTokyo uses custom-written PermitControl: https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/access/PermitControl.sol#L21

It is recommended to use OpenZeppelin AccessControl instead of custom-written PermitControl. This has several benefits, making it a preferable choice:

1. Security: OpenZeppelin's contracts have been extensively reviewed, tested, and audited by the community and security experts
2. Standardization: OpenZeppelin's libraries and contracts follow industry best practices and standard patterns.
3. Maintainability: OpenZeppelin actively maintains and updates its contracts to address any new issues or changes in the Ethereum ecosystem

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol

LOW 2

NeoTokyo uses a single general interface IGenericGetter to call different contracts even though their ABI is different:

• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L631
• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L638
• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L664
• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L903
• https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L949

Using a single IGenericGetter interface for all contracts instead of separate interfaces for each contract can be problematic for catching type-related errors.

LOW 3

There is type-unsafe comparison in withdraw():

uint8(_assetType) == 2

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

which should be replaced with

_assetType == AssetType.BYTES

LOW 4

There is loss of precision in some formulas in the NeoTokyo contract. Generally it is better to put the division operation in the and of the expression.

The line:

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

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

can be replaced with

uint256 points = (amount * 100 * lpPosition.multiplier) / (1e18 * _DIVISOR);

The line:

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

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

can be replaced with

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

Gas optimization 1

Constraints checks for enum are added automatically by the solidity compiler, so the following code is redundant and may be removed to save gas:

if (uint8(_assetType) > 4) {
	revert InvalidAssetType(uint256(_assetType));
}

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

Similarly, the following code may be simplified from:

if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
	revert InvalidAssetType(uint256(_assetType));
}

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

To:

if (_assetType == AssetType.BYTES) {
	revert InvalidAssetType(uint256(_assetType));
}

Gas optimization 2

The code which removes a position in _withdrawLP()...

if (lpPosition.amount == 0) {
	lpPosition.multiplier = 0;
}

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

...misses the removal of lpPosition.timelockEndTime:

lpPosition.timelockEndTime = 0;

Adding it will add an additional gas compensation for the caller.

Gas optimization 3

Using bytes32 instead of string for mappings can save gas, as bytes32 is cheaper to store and compare. You can change the identityCreditYield and vaultCreditMultiplier mappings to use bytes32 keys:

mapping(uint256 => mapping(bytes32 => string)) private identityCreditYield;
mapping(bytes32 => uint256) private vaultCreditMultiplier;

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L326 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L332

Make sure to update the contract code accordingly to work with bytes32 keys instead of string.

Gas optimization 4

Remove unnecessary else in getStakerPosition():

Replace the following code...

function getStakerPosition (
	address _staker,
	AssetType _assetType
) external view returns (uint256[] memory) {
	if (_assetType == AssetType.S1_CITIZEN) {
		return _stakerS1Position[_staker];
	} else if (_assetType == AssetType.S2_CITIZEN) {
		return _stakerS2Position[_staker];
	} else {
		revert UnknowablePosition(uint256(_assetType));
	}
}

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

by this:

function getStakerPosition (
	address _staker,
	AssetType _assetType
) external view returns (uint256[] memory) {
	if (_assetType == AssetType.S1_CITIZEN) {
		return _stakerS1Position[_staker];
	} else if (_assetType == AssetType.S2_CITIZEN) {
		return _stakerS2Position[_staker];
	}
	revert UnknowablePosition(uint256(_assetType));
}