Biconomy Hyphen 2.0 contest - berndartmueller's results

QA Report

Non-Critical Findings

Rename contract LiquidityProviders to LiquidityProvider

Description

It's confusing to use the plural for this contract name. Consider renaming the contract (and interface) to LiquidityProvider (singular).

Redundant type cast to payable

Description

Function argument _to is defined as address payable, the type casting to payable on L187 is redundant.

Findings

LiquidityFarming.sol#L187

function reclaimTokens(
  address _token,
  uint256 _amount,
  address payable _to
) external nonReentrant onlyOwner {
  require(_to != address(0), "ERR__TO_IS_ZERO");
  if (_token == NATIVE) {
    (bool success, ) = payable(_to).call{ value: _amount }(""); // @audit-info redundant type cast to `payable()`
    require(success, "ERR__NATIVE_TRANSFER_FAILED");
  } else {
    IERC20Upgradeable(_token).safeTransfer(_to, _amount);
  }
}

Recommended mitigation steps

Remove redundant type cast:

function reclaimTokens(
  address _token,
  uint256 _amount,
  address payable _to
) external nonReentrant onlyOwner {
  require(_to != address(0), "ERR__TO_IS_ZERO");
  if (_token == NATIVE) {
    (bool success, ) = _to.call{ value: _amount }(""); // @audit-info removed redundant type cast to `payable()`
    require(success, "ERR__NATIVE_TRANSFER_FAILED");
  } else {
    IERC20Upgradeable(_token).safeTransfer(_to, _amount);
  }
}

10**18 can be changed to 1e18

Description

For better readability, change 10**18 to 1e18.

Findings

LiquidityProviders.sol#L27

Use scientific notation 1e10 instead of 10000000000

Description

For better readability and to prevent any issues, use the scientific notation 1e10 instead of 10000000000

Findings

LiquidityPool.sol#L20

Reuse BASE_DIVISOR in LiquidityPool.getRewardAmount()

Description

Using large numbers with many zeros (e.g. 10000000000) can cause issues when accidentally having inconsistent number of zeros. I recommend to use the constant variable BASE_DIVISOR instead.

function getRewardAmount(uint256 amount, address tokenAddress) public view returns (uint256 rewardAmount) {
  uint256 currentLiquidity = getCurrentLiquidity(tokenAddress);
  uint256 providedLiquidity = liquidityProviders.getSuppliedLiquidityByToken(tokenAddress);
  if (currentLiquidity < providedLiquidity) {
    uint256 liquidityDifference = providedLiquidity - currentLiquidity;
    if (amount >= liquidityDifference) {
      rewardAmount = incentivePool[tokenAddress];
    } else {
      // Multiply by BASE_DIVISOR to avoid 0 reward amount for small amount and liquidity difference
      rewardAmount = (amount * incentivePool[tokenAddress] * BASE_DIVISOR) / liquidityDifference;
      rewardAmount = rewardAmount / BASE_DIVISOR;
    }
  }
}

Findings

LiquidityPool.sol#L184
LiquidityPool.sol#L185

Spelling mistakes

Description

Multiple spelling mistakes across contracts. Both in code (variables, functions,..) as well as in documentation.

Findings

hyphen/token/TokenManager.sol

L66: chainid -> chainId

hyphen/LiquidityProviders.sol

L75: initalizes -> initializes
L357: Claculate -> Calculate

hyphen/LiquidityFarming.sol

L64: Updation -> Update
L96: initalizeRewardPool -> initializeRewardPool
L114: recepientBalance -> recipientBalance
L178: reightful -> rightful
L264: comitted -> committed

hyphen/WhitelistPeriodManager.sol

L81 liqudity-> liquidity
L102 liqudity-> liquidity
L113 liqudity-> liquidity
L128 liqudity-> liquidity
L226 ERR__LENGTH_MISMACH -> ERR__LENGTH_MISSMATCH
L246 getMaxCommunityLpPositon -> getMaxCommunityLpPosition (Function name! Also correct spelling in interface)

hyphen/LiquidityPool.sol

L145 transfered -> transferred
L275 amnt -> amount
L300 afetr -> after

Low Risk

Prevent minting multiple NFTs per owner per LP

Description

In the README (see here) it states that if someone wants to increase the liquidity of an existing position, increaseLiquidity() should be called.

Recommended mitigation steps

I would advise to prevent adding liquidity via _addLiquidity() when there's already an existing position. Add a require() to check if there's already an existing position.

Findings

LiquidityProviders.sol#L238

function _addLiquidity(address _token, uint256 _amount) internal {
  require(_amount > 0, "ERR__AMOUNT_IS_0");
  require(lpToken.balanceOf(_msgSender()) == 0, "ERR_LIQUIDITY_ALREADY_ADDED"); // @audit-info Check if there's an existing LP position and prevent minting multiple NFTs for same owner per LP
  uint256 nftId = lpToken.mint(_msgSender());
  LpTokenMetadata memory data = LpTokenMetadata(_token, 0, 0);
  lpToken.updateTokenMetadata(nftId, data);
  _increaseLiquidity(nftId, _amount);
}

Unbound iteration over LP positions

Description

The WhitelistPeriodManager.getMaxCommunityLpPositon() function iterates over all LP tokens. As the amount of tokens is unbound and grows by each newly added LP position, it consumes more and more gas und possibly runs out of gas.

As this function is not called in critical situations, I consider this finding as rather low risk.

Recommended mitigation steps

I recommend to perform the calculation off-chain due to unbound for loop which can greatly increase in size.

Findings

WhitelistPeriodManager.sol#L245

Gas Optimizations

Unused state variable

Description

Unused state variable.

Findings

LPToken.NATIVE

Recommended mitigation steps

Remove unused state variables to safe gas.

Public functions that could be declared external to save gas

Description

Following functions should be declared external, as functions that are never called by the contract internally should be declared external to save gas.

Findings

ExecutorManager.getExecutorStatus()
ExecutorManager.getAllExecutors()
LiquidityFarming.setRewardPerSecond()
LiquidityFarming.getNftIdsStaked()
LiquidityFarming.getRewardRatePerSecond()
LiquidityPool.setTrustedForwarder()
LiquidityPool.setLiquidityProviders()
LiquidityPool.getExecutorManager()
LiquidityProviders.getTotalReserveByToken()
LiquidityProviders.getSuppliedLiquidityByToken()
LiquidityProviders.getTotalLPFeeByToken()
LiquidityProviders.getCurrentLiquidity()
LiquidityProviders.increaseCurrentLiquidity()
LiquidityProviders.decreaseCurrentLiquidity()
LiquidityProviders.getFeeAccumulatedOnNft()
LPToken.setSvgHelper()
LPToken.getAllNftIdsByUser()
LPToken.exists()
TokenManager.getEquilibriumFee()
TokenManager.getMaxFee()
TokenManager.getTokensInfo()
TokenManager.getDepositConfig()
TokenManager.getTransferConfig()

Recommended mitigation steps

Use the external attribute for functions never called from the contract.

> 0 is less efficient than != 0 for unsigned integers

Description

!= 0 is a cheaper (less gas) operation for unsigned integers within require statements compared to > 0.

Findings

LiquidityProviders.sol#L239
LiquidityProviders.sol#L283
LiquidityProviders.sol#L410

Recommended mitigation steps

Change > 0 to != 0.

Avoid long revert strings

Description

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Findings

ExecutorManager.sol#L17
LiquidityPool.sol#L77

Use unchecked {} primitive within for loops

Given the use of Solidity compiler >= 0.8.0, there are default arithmetic checks for mathematical operations which consume additional gas for such checks internally. In expressions where we are absolutely sure of no overflows/underflows, one can use the unchecked primitive to wrap such expressions to avoid checks and save gas.

Findings

ExecutorManager.sol#L31

Change to

for (uint256 i = 0; i < executorArray.length;) {
  addExecutor(executorArray[i]);

  unchecked { ++i; }
}

Also here:

ExecutorManager.sol#L47
LiquidityFarming.sol#L233
WhitelistPeriodManager.sol#L180
WhitelistPeriodManager.sol#L228
WhitelistPeriodManager.sol#L248
LPToken.sol#L77
TokenManager.sol#L78

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Recommended implementation

uint length = arr.length;

for (uint i; i < length; ++i) {
  // Operations not effecting the length of the array.
}

Findings

ExecutorManager.sol#L31
ExecutorManager.sol#L47
LiquidityFarming.sol#L233
WhitelistPeriodManager.sol#L180
WhitelistPeriodManager.sol#L228
WhitelistPeriodManager.sol#L248
LPToken.sol#L77
TokenManager.sol#78

Reorder require statements in LiquidityPool.sol:sendFundsToUser() to save gas on revert

Description

Require statements can be placed earlier to reduce gas usage on revert.

Findings

LiquidityPool.sol#L271

Recommended implementation

function sendFundsToUser(
    address tokenAddress,
    uint256 amount,
    address payable receiver,
    bytes memory depositHash,
    uint256 tokenGasPrice,
    uint256 fromChainId
) external nonReentrant onlyExecutor tokenChecks(tokenAddress) whenNotPaused {
    require(
        tokenManager.getTransferConfig(tokenAddress).min <= amount &&
            tokenManager.getTransferConfig(tokenAddress).max >= amount,
        "Withdraw amnt not in Cap limits"
    );
    require(receiver != address(0), "Bad receiver address");

    uint256 initialGas = gasleft();

    (bytes32 hashSendTransaction, bool status) = checkHashStatus(tokenAddress, amount, receiver, depositHash);

    ...
}

Reorder require statements in LiquidityProviders.sol:_increaseLiquidity() to save gas on revert

Description

Require statements can be placed earlier to reduce gas usage on revert.

Findings

LiquidityProviders.sol#L283

Recommended implementation

function _increaseLiquidity(uint256 _nftId, uint256 _amount) internal onlyValidLpToken(_nftId, _msgSender()) {
  require(_amount > 0, "ERR__AMOUNT_IS_0"); // @audit-info reorder require to top of function

  (address token, uint256 totalSuppliedLiquidity, uint256 totalShares) = lpToken.tokenMetadata(_nftId);

  whiteListPeriodManager.beforeLiquidityAddition(_msgSender(), token, _amount);

  ...
}

Reorder require statements in LiquidityProviders.sol:increaseNativeLiquidity() to save gas on revert

Description

Require statements can be placed earlier to reduce gas usage on revert.

Findings

LiquidityProviders.sol#L334

Recommended implementation

function increaseNativeLiquidity(uint256 _nftId) external payable nonReentrant whenNotPaused {
  require(_isSupportedToken(NATIVE), "ERR__TOKEN_NOT_SUPPORTED"); // @audit-info reorder require to top of function
  (address token, , ) = lpToken.tokenMetadata(_nftId);
  require(token == NATIVE, "ERR__WRONG_FUNCTION");
  (bool success, ) = address(liquidityPool).call{value: msg.value}("");

  ...
}

Remove unused variable access in modifier onlyValidLpToken()

Description

See @audit-info in following code snippet:

modifier onlyValidLpToken(uint256 _tokenId, address _transactor) {
    (address token, , ) = lpToken.tokenMetadata(_tokenId); // @audit-info not needed
    require(lpToken.exists(_tokenId), "ERR__TOKEN_DOES_NOT_EXIST");
    require(lpToken.ownerOf(_tokenId) == _transactor, "ERR__TRANSACTOR_DOES_NOT_OWN_NFT");
    _;
}

Findings

LiquidityProviders.sol#L54

Unused named returns can be removed

Description

Removing unused named return variables can reduce gas usage and improve code clarity.

Recommended mitigation steps

Remove the unused named return variables or use them instead of creating additional variables.

Findings

hyphen/token/TokenManager.sol

L139 unnecessary named return sender

hyphen/LiquidityProviders.sol

L457 unnecessary named return sender

hyphen/LiquidityFarming.sol

L351 unnecessary named return sender

hyphen/LiquidityPool.sol

L416 unnecessary named return sender