Biconomy Hyphen 2.0 contest - Cantor_Dust's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L255 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L175-L188

Vulnerability details

Impact

Rewards are given to a user for depositing either ERC20 tokens or their native token into the LiquidityPool. This reward is used to incentivize users to deposit funds into the liquidity pool when the pool is not in an equilibrium state.

For regular users, this liquidity pool state fluctuates based on the frequency and amount of deposits made to the liquidity pool. If a malicious user can control the state of the liquidity pool before a victim deposits tokens into the liquidity pool, they can gain double rewards.

To gain these double rewards, a malicious user can watch the mempool for transactions that will receive a reward when the deposit occurs. When a malicious user sees that victim deposit, the malicious user can attach a higher fee to their transaction and initiate a deposit. This will allow the malicious user's transaction to front-run before the victim's transaction.

Once the malicious user's deposit is complete, the liquidity pool state will be in a near equilibrium state. Then, the victim's deposit will occur which causes the liquidity pool state to no longer be in equilibrium.

Finally, the malicious user will make a final deposit gaining yet another reward for bringing the liquidity pool state back to equilibrium.

To sum up, a malicious user can create a sandwich attack where they deposit their own tokens before and after a victim's transaction. This will allow the malicious user to double dip and gain rewards twice due to victim's deposit.

Proof of Concept

Let's look at the depositNative function which is the simpler of the two deposit functions.

The key component in the depositNative function is the getRewardAmount which can be found here (https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L255) . The getRewardAmount calculates how much available vs supplied liquidity exists in the liquidity pool. There are no (https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L175-L188) time-weighted checks to calculate the available vs. supplied liquidity. With a lack of checks for time-weight and that there are no frontrun checks against deposits, it's trivial to front-run deposits and control the liquidity of the liquidity such that the reward amount can be double-dipped.

Tools Used

Text editor

Recommended Mitigation Steps

1. By allowing each deposit to manipulate the liquidity pool state from either a deficient or excessive state, malicious users can double dip on rewards.
2. Alternative approaches to calculating rewards is possible, for example a dutch auction style deposit system where rewards are distributed evenly could reduce an impact of a frontrun attack.
3. A simpler approach is to record liquidity states at specific block timestamps and check against the timestamp for the current block state.

updatePool allows does no checks against _baseToken #17

The HyphenLiquidityFarming#updatePool allows any address as the _baseToken argument. Anyone is allowed to create a pool value inside poolInfo. There are no safety checks that the address passed to the function matches a known poolInfo.

    /// @return pool Returns the pool that was updated.
    function updatePool(address _baseToken) public whenNotPaused returns (PoolInfo memory pool) {
        pool = poolInfo[_baseToken];
        if (block.timestamp > pool.lastRewardTime) {
            if (totalSharesStaked[_baseToken] > 0) {
                pool.accTokenPerShare = getUpdatedAccTokenPerShare(_baseToken);
            }
            pool.lastRewardTime = block.timestamp;
            poolInfo[_baseToken] = pool;
            emit LogUpdatePool(_baseToken, pool.lastRewardTime, totalSharesStaked[_baseToken], pool.accTokenPerShare);
        }

        // struct PoolInfo {
        //     uint256 accTokenPerShare;
        //     uint256 lastRewardTime;
        // }
    }

No validation on tokenConfig's min or max leading to broken functionality

File: TokenManager.sol

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

Affects: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L157

No zero address check for _to in HyphenLiquidityFarming

The following functions don't have address validation on _to:

• staking
• withdraw
• deposit
• extractRewards

changeFee function is missing tokenChecks modifier

The tokenChecks modifier is used on other onlyOwner functions aside from this one.

function changeFee(
        address tokenAddress,
        uint256 _equilibriumFee,
        uint256 _maxFee
    ) external override onlyOwner whenNotPaused {
        require(_equilibriumFee != 0, "Equilibrium Fee cannot be 0");
        require(_maxFee != 0, "Max Fee cannot be 0");
        tokensInfo[tokenAddress].equilibriumFee = _equilibriumFee;
        tokensInfo[tokenAddress].maxFee = _maxFee;
        emit FeeChanged(tokenAddress, tokensInfo[tokenAddress].equilibriumFee, tokensInfo[tokenAddress].maxFee);
    }

LiquidityPool.sol

LiquidityPool.sol::140 - uint256 liquidityPoolBalance = liquidityProviders.getCurrentLiquidity(tokenAddress);
(this extra MSTORE is unneeded. Since the variable is only used once just read directly from storage and save an MSTORE/MLOAD)
LiquidityPool.sol::525 -  if (transferFeePerc > tokenManager.getTokensInfo(tokenAddress).equilibriumFee) 
(multiple reads to tokenManager.getTokensInfo(tokenAddress).equilibriumFee, save as a local variable to save gas)

ExecutorManager.sol -

ExecutorManager.sol::l20 - require(executorStatus[msg.sender], "You are not allowed to perform this operation");
(Shorten revert string to <32 bytes)
ExecutorManager.sol::36 and ExecutorManager.sol::52 - for (uint256 i = 0; i < executorArray.length; ++i) {
(The increment in for loop post condition can be made unchecked. Array length is read each iteration - uses more gas than caching len outside of loop.)
           

LiquidityFarming.sol -

 LiquidityFarming.sol::32 -  can pack address and bool together to save 1 storage slot
 LiquidityFarming.sol::190 - (bool success, ) = payable(_to).call{value: _amount}("");
 (no need to recast address payable _to as payable again)
 LiquidityFarming.sol::224 -  totalSharesStaked[baseToken] += amount;
 (can be done in an unchecked block to save gas, no risk of overflow)
 LiquidityFarming.sol::286 - --i;
 (can be done in an unchecked block to save gas, no risk of underflow)

 
   
 LiquidityFarming.sol::135 - if (amount > 0) {
 LiquidityFarming.sol::321 - if (totalSharesStaked[_baseToken] > 0) {
 (This is disputed. != is a cheaper than > for unit comparision.)
 3 optims:
 LiquidityFarming.sol::236 - for (index = 0; index < nftsStakedLength; ++index) {
 (The increment in for loop post condition can be made unchecked. Array length is read each iteration - uses more gas than caching len outside of loop. != is a cheaper than > for unit comparision. (Compiler optimizes this, might remove.))

LiquidityProviders.sol -

  LiquidityProviders.sol::69 - modifier onlyValidLpToken(uint256 _tokenId, address _transactor) {
  (Putting lpToken in memory saves gas)
  LiquidityProviders.sol::182 - if (supply > 0) {
  LiquidityProviders.sol::239 - require(_amount > 0, "ERR__AMOUNT_IS_0");
  LiquidityProviders.sol::283 - require(_amount > 0, "ERR__AMOUNT_IS_0");
  LiquidityProviders.sol::410 - require(lpFeeAccumulated > 0, "ERR__NO_REWARDS_TO_CLAIM");
  (This is disputed. != is a cheaper than > for unit comparision. (Compiler optimizes this, might remove.))

TokenManager.sol -

TokenManager.sol::93 -  function setDepositConfig(uint256[] memory toChainId, address[] memory tokenAddresses,TokenConfig[] memory tokenConfig)
(Calldata can be used as the location instead since the array values are only read)
3 optims: TokenManager.sol::102 - for (uint256 index = 0; index < tokenConfig.length; ++index) {
(The increment in for loop post condition can be made unchecked. Intitializes with default unit256 value, uses more gas. (Compiler optimizes this, might remove) Array length is read each iteration - uses more gas than caching len outside of loop.)

WhitelistPeriodManager.sol -

3 optims:   
WhitelistPeriodManager.sol::270 - for (uint256 i = 0; i < _addresses.length; ++i) {
WhitelistPeriodManager.sol::323 - for (uint256 i = 0; i < _tokens.length; ++i) {
(The increment in for loop post condition can be made unchecked. Intitializes with default unit256 value, uses more gas.(Compiler optimizes this, might remove) Array length is read each iteration - uses more gas than caching len outside of loop.)
WhitelistPeriodManager.sol::356 - uint256 maxLp = 0;
(Shorten revert string to =< 32 bytes)

LPToken.sol -

3 optims:
LPToken.sol::142 - for (uint256 i = 0; i < nftIds.length; ++i) {
(The increment in for loop post condition can be made unchecked. Intitializes with default unit256 value, uses more gas. (Compiler optimizes this, might remove.) Array length is read each iteration - uses more gas than caching len outside of loop.)
LPToken.sol::87 - require(_whiteListPeriodManager != address(0), "ERR_INVALID_WHITELIST_PERIOD_MANAGER");
(Shorten revert string to =< 32 bytes)