Biconomy Hyphen 2.0 contest - sirhashalot's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/WhitelistPeriodManager.sol#L202-L208

Vulnerability details

Impact

The setPerTokenWalletCap() function in WhitelistPeriodManager.sol contains a comment stating:

Special care must be taken when calling this function
There are no checks for _perTokenWalletCap (since it's onlyOwner), but it's essential that it should be >= max lp provided by an lp.
Checking this on chain will probably require implementing a bbst, which needs more bandwidth
Call the view function getMaxCommunityLpPositon() separately before changing this value

Even if the manual step of calling the getMaxCommunityLpPositon() function is properly performed, it is possible for a user to add liquidity to increase the maxLp value in between when the getMaxCommunityLpPositon() function is called and when the setPerTokenWalletCap() function is called. Because this process is manual, this doesn't need to be bot frontrunning in the same block as when the setPerTokenWalletCap() function is called, but can be cause by poor timing of an innocent unknowing user adding liquidity to the protocol. If this condition occurs, the liquidity provider will have provided more liquidity than the perTokenWalletCap limit, breaking the assumptions for this variable and leading to some denial of service conditions.

This edge situation can impact the setTotalCap() function and the "perTokenTotalCap[_token]" state variable as well, but the "perTokenWalletCap[_token]" value would have to be reduced before the "perTokenTotalCap[_token]" value is reduced. The impact to setTotalCap() follows the same execution path but adds the additional step of calling the setTotalCap() function at the end of the process.

Proof of Concept

1. Owner calls getMaxCommunityLpPositon(_token) function to identify maxLp value to confirm new perTokenWalletCap value is below maxLp value
2. An innocent user adds liquidity to their position without the knowledge that the owner is going to reduce the "perTokenWalletCap[_token]" value soon
3. Owner calls setPerTokenWalletCap() function to reduce "perTokenWalletCap[_token]" value
4. The innocent user has more liquidity than the new "perTokenWalletCap[_token]" value. This means that the user can be in a situation where if they remove x amount of liquidity and attempt to add x liquidity back to their position, the innocent user will be unable to do so. Other functions that rely on the assumption that the largest user deposit is below the "perTokenWalletCap[_token]" value may break due to incorrect assumptions

This edge situation can impact the setTotalCap() function and the "perTokenTotalCap[_token]" state variable as well, but the "perTokenWalletCap[_token]" value would have to be reduced before the "perTokenTotalCap[_token]" value is reduced. The impact to setTotalCap() follows the same execution path but adds the additional step of calling the setTotalCap() function at the end of the process.

Tools Used

Manual analysis

Recommended Mitigation Steps

A programmatic solution is the only way to avoid these edge case scenarios, though it will increase gas consumption. To convert the manual calling of getMaxCommunityLpPositon(_token) to a programmatic solution, add the following require statement next to the existing require statement of the setPerTokenWalletCap() function: require(_perTokenWalletCap <= getMaxCommunityLpPositon(_token), "ERR_PWC_GT_MCLP");

1. Remove unnecessary import

Impact

The IERC20Upgradeable.sol is imported twice in LiquidityPool.sol, once from the SafeERC20Upgradeable.sol import and once from the direct import of IERC20Upgradeable.sol. The direct import line can be removed.

Proof of Concept

IERC20Upgradeable.sol is imported indirectly on line 9 and imported directly on line 10 of LiquidityPool.sol.

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the following line from LiquidityPool.sol:

import   "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";

2. Add unchecked

Impact

When an overflow or underflow is not possible, unchecked can be used to save the gas consumed by SafeMath in Solidity 0.8.0+

Proof of Concept

LiquidityPool.sol contains this code:

if (currentLiquidity < providedLiquidity) {
 uint256 liquidityDifference = providedLiquidity - currentLiquidity;

Because providedLiquidity - currentLiquidity cannot underflow, unchecked can be used

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the line of code copied above with the following

if (currentLiquidity < providedLiquidity) {
  unchecked { uint256 liquidityDifference = providedLiquidity - currentLiquidity; }

3. Skip setting default int value

Impact

When an overflow or underflow is not possible, unchecked can be used to save the gas consumed by SafeMath in Solidity 0.8.0+

Proof of Concept

LiquidityPool.sol contains this code:

if (currentLiquidity < providedLiquidity) {
 uint256 liquidityDifference = providedLiquidity - currentLiquidity;

Because providedLiquidity - currentLiquidity cannot underflow, unchecked can be used

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the line of code copied above with the following

if (currentLiquidity < providedLiquidity) {
  unchecked { uint256 liquidityDifference = providedLiquidity - currentLiquidity; }

4. Revert string > 32 bytes

Impact

Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas as documented publicly

Proof of Concept

The locations where a default int value is set are listed below.

1. LPToken.sol Line 70
2. LPToken.sol Line 77

Tools Used

Manual analysis

Recommended Mitigation Steps

Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, the code could be modified to use custom errors, introduced in Solidity 0.8.4: https://blog.soliditylang.org/2021/04/21/custom-errors/

5. Public functions can be external

Impact

Many functions can be declared external for gas savings

Proof of Concept

The functions that can benefit from this gas optimization and their corresponding contract are listed below.

ExecutorManager.sol

• getExecutorStatus
• getAllExecutors

LiquidityFarming.sol

• initialize
• setRewardPerSecond
• getNftIdsStaked
• getRewardRatePerSecond

LiquidityPool.sol

• initialize
• setTrustedForwarder
• setLiquidityProviders
• getExecutorManager

LiquidityProviders.sol

• initialize
• getTotalReserveByToken
• getSuppliedLiquidityByToken
• getTotalLPFeeByToken
• getCurrentLiquidity
• increaseCurrentLiquidity
• decreaseCurrentLiquidity
• getFeeAccumulatedOnNft

WhitelistPeriodManager.sol

• initialize

LPToken.sol

• initialize
• setSvgHelper
• getAllNftIdsByUser
• exists

TokenManager.sol

• getEquilibriumFee
• getMaxFee
• getTokensInfo
• getDepositConfig
• getTransferConfig

Tools Used

Slither

Recommended Mitigation Steps

Declare functions as external instead of public when possible for gas savings

6. Unnecessary uint zero initialization

Impact

uint256 variables are initialized to a default value of zero per Solidity docs: https://docs.soliditylang.org/en/latest/control-structures.html#default-value

Setting a variable to the default value is unnecessary. Shortening or removing lines of code where variables are initialized to zero can save gas. Here are a few articles describing this gas optimization: https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#53bd https://medium.com/coinmonks/gas-optimization-in-solidity-part-i-variables-9d5775e43dde#4135

Proof of Concept

The locations where a default int value is set are listed below.

1. Line 266 of LiquidityFarming.sol
2. Line 247 of WhitelistPeriodManager.sol

Tools Used

Manual analysis

Recommended Mitigation Steps

Instead of initializing a variable to zero, such as uint256 abc = 0;, the line can be shortened to uint256 abc; as Solidity automatically initializes uint variables to zero.