Biconomy Hyphen 2.0 contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L140 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L145 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L187

Vulnerability details

Low level calls (call, delegate call and static call) return success if the called contract doesn’t exist (not deployed or destructed)

This makes a user be able to send his funds to non-existing addresses.

LiquidityFarming reclaimTokens - if the owner calls by accident with a non-existing address he'll lose the funds. _sendRewardsForNft - if the withdraw or extractRewards will be called with a to non-existing address, the funds will be lost. That's because of the call to _sendRewardsForNft which contains a low level call to the to address.

sendFundsToUser - if an executor calls by accident with a non-existing address the funds will be lost. transfer - if the transfer function will be called (by the LiquidityProvidors contract of course) with a non existing address as a receiver, the funds will be lost.

This can be seen here https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf (report #9) and here https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L180-L186

Vulnerability details

Impact

The getTokenPriceInLPShares function calculates the token price in LP shares, but it checks a wrong condition - if supposed to return BASE_DIVISOR if the total reserve is zero, not if the total shares minted is zero. This might leads to a case where the price is calculated incorrectly, or a division by zero is happening.

Proof of Concept

This is the wrong function implementation:

function getTokenPriceInLPShares(address _baseToken) public view returns (uint256) {
    uint256 supply = totalSharesMinted[_baseToken];
    if (supply > 0) {
        return totalSharesMinted[_baseToken] / totalReserve[_baseToken];
    }
    return BASE_DIVISOR;
}

This function is used in this contract only in the removeLiquidity and claimFee function, so it's called only if funds were already deposited and totalReserve is not zero, but it can be problematic when other contracts will use this function (it's a public view function so it might get called from outside of the contract).

Recommended Mitigation Steps

The correct code should be:

function getTokenPriceInLPShares(address _baseToken) public view returns (uint256) {
    uint256 reserve = totalReserve[_baseToken];
    if (reserve > 0) {
        return totalSharesMinted[_baseToken] / totalReserve[_baseToken];
    }
    return BASE_DIVISOR;
}

Lines of code

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

Vulnerability details

Impact

In case of totalReserve[token] != 0 and totalSharesMinted[token] == 0, then mintedSharesAmount will be equal to 0, and therefore the require - require(mintedSharesAmount >= BASE_DIVISOR, "ERR__AMOUNT_BELOW_MIN_LIQUIDITY"); will always fail, so denial of service.

Proof of Concept

When depositor calls addTokenLiquidity, he make the totalReserve[token] and totalSharesMinted[token] to become bigger. then, another depositor will do that. Then 1 of them can withdraw and make the totalSharesMinted[token] will be 0, and this is bug as I explained before.

Tools Used

vscode

Recommended Mitigation Steps

check that totalSharesMinted[token] won't be 0, in the _decreaseCurrentLiquidity function.

Low and Non-Critical

addSupportedToken not checking equilibriumFee and maxFee are not zero in TokenManager

The addSupportedToken in the TokenManager contract doesn't check that equilibriumFee and maxFee are not zero, which allows the owner to add a token with zero fees (we can see that this is an unwanted behavior because in the changeFee function these conditions are checked).

depositErc20 and depositNative - didn't write about tag parameter in the comment

This is not a bug, just a documentation mistake - the comment describing the depositErc20 and depositNative functions doesn't have an explanation about the tag parameter

there are ways to transfer eth to the contract without emitting the event EthRecieved

In multiple contract the implementation of the receive function is the following implementation:

receive() external payable {
    emit EthReceived(_msgSender(), msg.value);
}

This implementations emits an event every time ETH is received. However, there is a way to transfer ETH to the contract without emitting this event - this can be done using the selfdestruct function. selfdestruct sends all remaining ETH stored in the contract to a designated address, without calling the receive function. This can't be exploited, but if there will be a future use in these events, it is worth considering.

typo in sendFundsToUser:

There's a typo in the error message in the require in the sendFundsToUser function (wrote "amnt" instead of "amount").

require(
    tokenManager.getTransferConfig(tokenAddress).min <= amount &&
        tokenManager.getTransferConfig(tokenAddress).max >= amount,
    "Withdraw amnt not in Cap limits"
);

in depositErc20 - no check that token address != NATIVE

The depositErc20 doesn't have any check that the given token address doesn't equal to the NATIVE address (in this case the user should use the depositNative function). This is a needed check, it can be seen also in the withdrawErc20GasFee, so it needs to be applied in the depositErc20 function too.

didn't handle the edge case of eligibleLiquidity < nftSuppliedLiquidity in claimFee function

The edge case of eligibleLiquidity < nftSuppliedLiquidity is handled in multiple functions, for example in the removeLiquidity the assignment is divided as the following:

if(nftSuppliedLiquidity > eligibleLiquidity) {
    lpFeeAccumulated = 0;
} else {
    unchecked {
        lpFeeAccumulated = eligibleLiquidity - nftSuppliedLiquidity;
    }
}

This is done to prevent underflow in the case where eligibleLiquidity < nftSuppliedLiquidity. However, this edge case is not handled in the claimFee function, where the assignment is simply done by uint256 lpFeeAccumulated = eligibleLiquidity - nftSuppliedLiquidity. This might be a desired underflow, because you need positive lpFeeAccumulated in order to claim the fees, however I think that an clear error message is better. This can be done by replacing this code:

uint256 lpFeeAccumulated = eligibleLiquidity - nftSuppliedLiquidity;
require(lpFeeAccumulated > 0, "ERR__NO_REWARDS_TO_CLAIM");

with this code:

require(eligibleLiquidity > nftSuppliedLiquidity, "ERR__NO_REWARDS_TO_CLAIM");
unchecked {
    uint256 lpFeeAccumulated = eligibleLiquidity - nftSuppliedLiquidity;
}

This will give the error message also in the case where eligibleLiquidity < nftSuppliedLiquidity and won't just revert on the case of underflow.

the getMaxCommunityLpPositon function can be DoS by minting many NFTs (by adding small liquidity) - can happen only if the min is small enough to allow minting that much NFTs

An attacker can add and remove liquidity to the pool and mint many NFTs, so that the token id number will be high enough in order to DoS the getMaxCommunityLpPositon function (which iterates through all the token ids until the last token id created). My suggestion is to make a similar function to getMaxCommunityLpPositon that receives an array of token ids and return the maximum liquidity that a single id has, so that the caller can avoid iterating through all the useless ids that an attacker can create. This can also be done by a function getting a range of token ids and iterating only in that range. Another approach to this will be to save a maximum liquidity variable and update it on every change to the liquidity pool. This solution adds some gas to the liquidity actions, but it fixes the problem.

LiquidityPoolProxy on different pragma version (it's on 0.8.2 while all the other contract are on 0.8.0)

The LiquidityPoolProxy's pragma version is 0.8.2 while all the other contract are on 0.8.0.

A mistake in a comment

The comment in the setRewardPerSecond says that the function sets the "amount of sushi per second", however this should be "amount of BICO per second" instead.

/// @notice Sets the sushi per second to be distributed. Can only be called by the owner.
/// @param _rewardPerSecond The amount of Sushi to be distributed per second.
function setRewardPerSecond(address _baseToken, uint256 _rewardPerSecond) public onlyOwner {
    rewardRateLog[_baseToken].push(RewardsPerSecondEntry(_rewardPerSecond, block.timestamp));
    emit LogRewardPerSecond(_baseToken, _rewardPerSecond);
}

A mistake in another comment

The comment of the reclaimTokens function says to use 0x00 as the token for Ethereum, however this is not correct - the value that should be used is 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE.

...
/// @param _token Token to reclaim, use 0x00 for Ethereum
...
function reclaimTokens(...) { ... }

missing parameter in function comment

The deposit function has a _to parameter that is not written in the comment

/// @notice Deposit LP tokens
/// @param _nftId LP token nftId to deposit.
function deposit(uint256 _nftId, address payable _to) external whenNotPaused nonReentrant { 
    ... 
}

wrong parameter passed to the GasFeeWithdraw event

The withdrawNativeGasFee function emits a GasFeeWithdraw event. It's first argument, as we can see in its definition, is the token address: event GasFeeWithdraw(address indexed tokenAddress, address indexed owner, uint256 indexed amount) However, the withdrawNativeGasFee emits that event with address(this) as the token address, instead of NATIVE which is a mistake.

tokenChecks modifier is not called in depositNative

The totalChecks modifier is not called in depositNative, which might cause a situation when we don't want the native token to be supported but it would still be deposit-able.

Not checking the allowance in depositErc20

in the depositErc20 function, you don't check the allowance to the user before calling to safeTransferFrom. This check is done in multiple functions in the LiquidityProviders contract, like addTokenLiquidity and increaseTokenLiquidity, so it might be wanted here too (or redundant in the other functions).

Front-runnable initialize (making the contract must be redeployed)

The initialize function in multiple contracts is front-runnable, which means that if an attacker manages to front run the original transaction that calls initialize, he can put wrong parameter values and claim the ownership of the contract, which basically makes the contract must be redeployed. This can be fixed using access controls for example, that can be initialized in the constructor.

getters for public fields like totalReserve, totalLiquidity, currentLiquidity, totalLPFees, totalSharesMinted are created automatically

Because of the mentioned variables are public, a getter is created for them automatically, which makes functions like getTotalReserveByToken, getSuppliedLiquidityByToken, getTotalLPFeeByToken, getCurrentLiquidity redundant.

forgot to remove the commented storage variable in LiquidityFarming

This variable is commented and should be removed (it's saved in the rewardRateLog variable).

/// @notice Reward rate per base token
//mapping(address => uint256) public rewardPerSecond;

Gas Optimizations

LiquidityFarming storage optimization

Use a mapping to bool instead of putting the bool inside the struct will save space, in addition the mapping uint to bool can be optimized using uint=>uint mapping and bitwise operations. In the LiquidityFarming contract the following contract is defined:

struct NFTInfo {
    address payable staker;
    uint256 rewardDebt;
    uint256 unpaidRewards;
    bool isStaked;
}

and a storage variable that maps an NFT token id to a NFTInfo variable. As you probably know, a storage slot in solidity is 256 bits (32 bytes), so the bool isStaked actually takes 32 bytes - but it only saves 1 bit for every token id, so it turns out that we have 255 unused bits for every token id in the mapping!

Let's see how we can optimize this. Instead of saving a mapping from a token id to NFTInfo, we'll define it a little be different.

First, let's define the struct without the boolean field:

struct NFTInfo {
    address payable staker;
    uint256 rewardDebt;
    uint256 unpaidRewards;
}

Now, let's define our mapping, but now we will use 2 mappings.

mapping(uint256 => NFTInfo) nftInfo;
mapping(uint256 => uint256) isStaked;

Now the nftInfo will map a token id to a NFTInfo variable. The second isStaked mapping will give us the information about the isStaked boolean variable. It will work using bitwise operations.

Each token id will be mapped to a single bit of a uint256 - so that 256 token ids boolean field will be saved in one uint256 variable. The bit that corresponds to token id = i is the i % 256th bit of the isStaked[i / 256] variable.

Helper functions to get and set the boolean values will look like this:

// returns true if the token id is staked, false otherwise
function getIsStaked(uint256 tokenId) public (returns bool) {
    uint mask = 1 << (tokenId % 256);
    return (isStaked[tokenId / 256] & mask) != 0;
}

// sets the is staked value of token id to isTokenStaked
function setIsStaked(uint256 tokenId, bool isTokenStaked) public {
    if (isTokenStaked) {
        uint mask = 1 << (tokenId % 256);
        isStaked[tokenId / 256] = isStaked[tokenId / 256] | mask;
    } else {
        uint mask = ~(1 << (tokenId % 256));
        isStaked[tokenId / 256] = isStaked[tokenId / 256] & mask;
    }
}

This optimization will save a lot of storage - from 256 bits for every token id to 1 bit for every token id.

You can see this optimization explained in this article

Optimizations of for loops

Loops can be optimized in many cases, and here too. There are many optimizations that can be done. I'll write about them, and then show different loops from the code and will tell how they can be optimized, using the optimizations I explained before:

1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So the code can be optimized using uint i; instead of uint i = 0;.
2. Use ++i instead of i++ to save some gas spent in every iteration (this is cheaper and does the same in this case).
3. Use unchecked on the loop index incrementing.
4. Save the array length in a local variable before the loop instead of accessing it in every iteration.
5. Save one or more array variables that are being accessed in a local variable instead of accessing the ith element of the array multiple times in every iteration.

Now let's look at some code:

• addExecutor and removeExecutors functions in ExecutorManager contract Here we can use optimizations 1, 3 and 4 from the optimizations I explained before. The code will look like:

  //Register new Executors
  function addExecutors(address[] calldata executorArray) external override onlyOwner {
      for (uint256 i = 0; i < executorArray.length; ++i) {
          addExecutor(executorArray[i]);
      }
  }
  
  //Remove registered Executors
  function removeExecutors(address[] calldata executorArray) external override onlyOwner {
      for (uint256 i = 0; i < executorArray.length; ++i) {
          removeExecutor(executorArray[i]);
      }
  }

• setDepositConfig function in TokenManager contract Here we can use optimizations 1, 3, 4 and 5 (save depositConfig[toChainId[index]][tokenAddresses[index]] as a storage variable and tokenConfig[index] as a memory variable)

  function setDepositConfig(
      uint256[] memory toChainId,
      address[] memory tokenAddresses,
      TokenConfig[] memory tokenConfig
  ) external onlyOwner {
      require(
          (toChainId.length == tokenAddresses.length) && (tokenAddresses.length == tokenConfig.length),
          " ERR_ARRAY_LENGTH_MISMATCH"
      );
      for (uint256 index = 0; index < tokenConfig.length; ++index) {
          depositConfig[toChainId[index]][tokenAddresses[index]].min = tokenConfig[index].min;
          depositConfig[toChainId[index]][tokenAddresses[index]].max = tokenConfig[index].max;
      }
  }

• getAllNftIdsByUser function in LPToken contract Here we can use optimizations 1, 3 and 4.

  function getAllNftIdsByUser(address _owner) public view returns (uint256[] memory) {
      uint256[] memory nftIds = new uint256[](balanceOf(_owner));
      for (uint256 i = 0; i < nftIds.length; ++i) {
          nftIds[i] = tokenOfOwnerByIndex(_owner, i);
      }
      return nftIds;
  }

• getUpdatedAccTokenPerShare function of the LiquidityFarming contract Here we can only add unchecked to the --i operation (optimization 3).

  function getUpdatedAccTokenPerShare(address _baseToken) public view returns (uint256) {
      uint256 accumulator = 0;
      uint256 lastUpdatedTime = poolInfo[_baseToken].lastRewardTime;
      uint256 counter = block.timestamp;
      uint256 i = rewardRateLog[_baseToken].length - 1;
      while (true) {
          if (lastUpdatedTime >= counter) {
              break;
          }
          unchecked {
              accumulator +=
                  rewardRateLog[_baseToken][i].rewardsPerSecond *
                  (counter - max(lastUpdatedTime, rewardRateLog[_baseToken][i].timestamp));
          }
          counter = rewardRateLog[_baseToken][i].timestamp;
          if (i == 0) {
              break;
          }
          --i;
      }
  
      // We know that during all the periods that were included in the current iterations,
      // the value of totalSharesStaked[_baseToken] would not have changed, as we only consider the
      // updates to the pool that happened after the lastUpdatedTime.
      accumulator = (accumulator * ACC_TOKEN_PRECISION) / totalSharesStaked[_baseToken];
      return accumulator + poolInfo[_baseToken].accTokenPerShare;
  }

• getMaxCommunityLpPositon function in WhitelistPeriodManager contract Here we can use only optimization 3.

  function getMaxCommunityLpPositon(address _token) external view returns (uint256) {
      uint256 totalSupply = lpToken.totalSupply();
      uint256 maxLp = 0;
      for (uint256 i = 1; i <= totalSupply; ++i) {
          uint256 liquidity = totalLiquidityByLp[_token][lpToken.ownerOf(i)];
          if (liquidity > maxLp) {
              maxLp = liquidity;
          }
      }
      return maxLp;
  }

• setIsExcludedAddressStatus function in WhitelistPeriodManager contract Here we can use optimizations 1, 3, 4 and 5 (save _addresses[i] and _status[i] as local variables)

  function setIsExcludedAddressStatus(address[] memory _addresses, bool[] memory _status) external onlyOwner {
      require(_addresses.length == _status.length, "ERR__LENGTH_MISMATCH");
      for (uint256 i = 0; i < _addresses.length; ++i) {
          isExcludedAddress[_addresses[i]] = _status[i];
          emit ExcludedAddressStatusUpdated(_addresses[i], _status[i]);
      }
  }

• setCaps function in WhitelistPeriodManager contract Here we can use optimizations 1, 3 and 4.

  function setCaps(
        address[] memory _tokens,
        uint256[] memory _totalCaps,
        uint256[] memory _perTokenWalletCaps
    ) external onlyOwner {
        require(
            _tokens.length == _totalCaps.length && _totalCaps.length == _perTokenWalletCaps.length,
            "ERR__LENGTH_MISMACH"
        );
        for (uint256 i = 0; i < _tokens.length; ++i) {
            setCap(_tokens[i], _totalCaps[i], _perTokenWalletCaps[i]);
        }
    }

save return values of functions

Return values of functions can be saved instead of calling the function multiple times. This can be done in multiple places in the code:

1. depositErc20 and depositNative and sendFundsToUser functions in LiquidityPool - don't call the same function twice (calling the tokenManager.getDepositConfig(..) function in the require)

   in depositErc20:

   require(
       tokenManager.getDepositConfig(toChainId, tokenAddress).min <= amount &&
           tokenManager.getDepositConfig(toChainId, tokenAddress).max >= amount,
       "Deposit amount not in Cap limit"
   );

   in depositNative:

   require(
       tokenManager.getDepositConfig(toChainId, NATIVE).min <= msg.value &&
           tokenManager.getDepositConfig(toChainId, NATIVE).max >= msg.value,
       "Deposit amount not in Cap limit"
   );

   in sendFundsToUser:

   require(
       tokenManager.getTransferConfig(tokenAddress).min <= amount &&
           tokenManager.getTransferConfig(tokenAddress).max >= amount,
       "Withdraw amnt not in Cap limits"
   );

2. getAmountToTransfer function in the LiquidityPool contract calls to a function multiple times (both to getTokensInfo and _msgSender())

3. The return value of getTokenPriceInLPShares in removeLiquidity can be saved instead of calling it twice, and this function can also be inlined instead of being called.

4. withdrawErc20GasFee and withdrawNativeGasFee (in the LiquidityPool contract) calls to _msgSender() multiple times

Add unchecked in the liquidityDifference calculation in getRewardAmount() function in LiquidityPool

function getRewardAmount(uint256 amount, address tokenAddress) public view returns (uint256 rewardAmount) {
    uint256 currentLiquidity = getCurrentLiquidity(tokenAddress);
    uint256 providedLiquidity = liquidityProviders.getSuppliedLiquidityByToken(tokenAddress);
    if (currentLiquidity < providedLiquidity) {
        // @audit can add here unchecked because we know that result will be positive
        uint256 liquidityDifference = providedLiquidity - currentLiquidity;
        if (amount >= liquidityDifference) {
            rewardAmount = incentivePool[tokenAddress];
        } else {
            // Multiply by 10000000000 to avoid 0 reward amount for small amount and liquidity difference
            rewardAmount = (amount * incentivePool[tokenAddress] * 10000000000) / liquidityDifference;
            rewardAmount = rewardAmount / 10000000000;
        }
    }
}

Use local variable instead of storage variable

The LiquidityProviders's getTokenPriceInLPShares function can use supply instead of accessing the totalSharesMinted mapping again.

function getTokenPriceInLPShares(address _baseToken) public view returns (uint256) {
    uint256 supply = totalSharesMinted[_baseToken];
    if (supply > 0) {
        // @audit use supply instead of totalSharesMinted[_baseToken]
        return totalSharesMinted[_baseToken] / totalReserve[_baseToken]; 
    }
    return BASE_DIVISOR;
}

_increaseLiquidity function - can avoid access a mapping multiple times

The values of totalSharesMinted[token] and totalReserve[token] can be saved as local variables in order to save some gas (they are accessed multiple times).

save nftIdsStaked[msgSender] as a storage variable in withdraw function in LiquidityFarming

nftIdsStaked[msgSender] is accessed multiple times through the function, and saving it as a local storage variable can save some gas (It is accessed in every iteration). The new code will look something like that:

function withdraw(uint256 _nftId, address payable _to) external whenNotPaused nonReentrant {
    address msgSender = _msgSender();
    uint[] storage idsStaked = nftIdsStaked[msgSender];
    uint256 nftsStakedLength = idsStaked.length;
    uint256 index;
    for (index = 0; index < nftsStakedLength; ++index) {
        if (idsStaked[index] == _nftId) {
            break;
        }
    }

    require(index != nftsStakedLength, "ERR__NFT_NOT_STAKED");
    idsStaked[index] = idsStaked[idsStaked.length - 1];
    idsStaked.pop();

    _sendRewardsForNft(_nftId, _to);
    delete nftInfo[_nftId];

    (address baseToken, , uint256 amount) = lpToken.tokenMetadata(_nftId);
    amount /= liquidityProviders.BASE_DIVISOR();
    totalSharesStaked[baseToken] -= amount;

    lpToken.safeTransferFrom(address(this), msgSender, _nftId);

    emit LogWithdraw(msgSender, baseToken, _nftId, _to);
}

save rewardTokens[baseToken] in a local variable in _sendRewardsForNft function in LiquidityFarming

rewardTokens[baseToken] can be saved in a local to save some gas, because it is being accessed twice (if entering the else section).

save rewardRateLog[_baseToken][i] and rewardRateLog[_baseToken][i].timestamp in getUpdatedAccTokenPerShare in LiquidityFarming

rewardRateLog[_baseToken][i] can be save in a local variable instead of being accessed multiple times to save gas. In addition, rewardRateLog[_baseToken][i].timestamp can be saved instead of being accessed twice.

inline functions to save gas

Short functions can be inlined, which means replacing the call to these functions with the actual function code (or similar code with the same logic), in order to save the gas spent on the function call. There are multiple short functions that can be inlined:

• The _setTokenManager, _increaseCurrentLiquidity, _decreaseCurrentLiquidity and _transferFromLiquidityPool functions from the LiquidityProviders contract.
• The _setTokenManager, _setLiquidityProviders, _setLpToken and the call to setCap in setCaps (it can be reaplaced by calls to the 2 functions that setCap calls to ) function in the WhitelistPeriodManager contract.

The ifEnabled function can be simplified

This function checks a condition, which can be simplified to a simpler condition:

old code:

function ifEnabled(bool _cond) private view returns (bool) {
    return !areWhiteListRestrictionsEnabled || (areWhiteListRestrictionsEnabled && _cond);
}

new code:

function ifEnabled(bool _cond) private view returns (bool) {
    return !areWhiteListRestrictionsEnabled || _cond;
}

A redundant require

The first require in the call to tokenChecks(NATIVE) in the LiquidityProviders contract is redundant (in the addNativeLiquidity function for example), because we know for sure that NATIVE != 0. You can create a new modifier for token checks where we know for sure that the token is not zero address, or divide the tokenChecks modifier into 2 different modifiers, one will check that the address is not zero and the second will check that it is indeed supported. Another solution will be to only call the _isSupportedToken function instead of using the modifier

Move the require to the else instead of checking it in both cases

In the _increaseLiquidity function, the require after the if statement needs to be checked only in the "else" case, because we know that _amount > 0, so BASE_DIVISOR * _amount >= BASE_DIVISOR. The only case where this can be false is in the "else" case, where mintedSharesAmount = (_amount * totalSharesMinted[token]) / totalReserve[token] so it might be that mintedSharesAmount < BASE_DIVISOR.

old code:

require(_amount > 0, "ERR__AMOUNT_IS_0");
whiteListPeriodManager.beforeLiquidityAddition(_msgSender(), token, _amount);

uint256 mintedSharesAmount;
// Adding liquidity in the pool for the first time
if (totalReserve[token] == 0) {
    mintedSharesAmount = BASE_DIVISOR * _amount;
} else {
    mintedSharesAmount = (_amount * totalSharesMinted[token]) / totalReserve[token];
}

require(mintedSharesAmount >= BASE_DIVISOR, "ERR__AMOUNT_BELOW_MIN_LIQUIDITY");

new code:

require(_amount > 0, "ERR__AMOUNT_IS_0");
whiteListPeriodManager.beforeLiquidityAddition(_msgSender(), token, _amount);

uint256 mintedSharesAmount;
// Adding liquidity in the pool for the first time
if (totalReserve[token] == 0) {
    mintedSharesAmount = BASE_DIVISOR * _amount;
} else {
    mintedSharesAmount = (_amount * totalSharesMinted[token]) / totalReserve[token];
    // @audit this line has moved to the "else" case
    require(mintedSharesAmount >= BASE_DIVISOR, "ERR__AMOUNT_BELOW_MIN_LIQUIDITY"); 
}

add the NFT meta data to the arguments of _burnSharesFromNft in LiquidityProviders to avoid accessing the tokenMetadata mapping of LPToken again

When calling the _burnSharesFromNft function, the token address is passed as a parameter, and the function accesses the lpToken.tokenMetadata mapping. However, this exact mapping element is accessed right before the calls to _burnSharesFromNft, so instead of accessing the mapping twice, those variables can be passed as parameters too.

Let's take the claimFee function for example:

(address _tokenAddress, uint256 nftSuppliedLiquidity, uint256 totalNFTShares) = lpToken.tokenMetadata(_nftId);
...
_burnSharesFromNft(_nftId, lpSharesRepresentingFee, 0, _tokenAddress);

Now in the call to _burnSharesFromNft this mapping element is accessed again, as we can see here:

(, uint256 nftSuppliedLiquidity, uint256 nftShares) = lpToken.tokenMetadata(_nftId);

The same thing happens in the removeLiquidity function (and these are the only calls to _burnSharesFromNft).

withdraw function in LiquidityFarming - use saved length

Use the saved array length instead of accessing the array's length again.

old code:

require(index != nftsStakedLength, "ERR__NFT_NOT_STAKED");
nftIdsStaked[msgSender][index] = nftIdsStaked[msgSender][nftIdsStaked[msgSender].length - 1];

new code:

require(index != nftsStakedLength, "ERR__NFT_NOT_STAKED");
nftIdsStaked[msgSender][index] = nftIdsStaked[msgSender][nftsStakedLength - 1];