Biconomy Hyphen 2.0 contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L151

Vulnerability details

Impact

The depositErc20 function allows setting tokenAddress = NATIVE and does not throw an error. No matter the amount chosen, the SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(tokenAddress), sender, address(this), amount); call will not revert because it performs a low-level call to NATIVE = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE, which is an EOA, and the low-level calls to EOAs always succeed. Because the safe* version is used, the EOA not returning any data does not revert either.

This allows an attacker to deposit infinite native tokens by not paying anything. The contract will emit the same Deposit event as a real depositNative call and the attacker receives the native funds on the other chain.

Recommended Mitigation Steps

Check tokenAddress != NATIVE in depositErc20.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L322

Vulnerability details

Impact

When updating the incentivePool it divides the previous value by BASE_DIVISOR. On each update, the incentivePool basically resets itself to only the increment and loses the previous incentive pool.

// @audit divides entire previous incentivePool by BASE_DIVISOR? wrong parenthesis
incentivePool[tokenAddress] =
  (incentivePool[tokenAddress] +
      // @audit-info wants to add excess fee (fee - eq) * amount to incentive, eq*amount is lpFee
      (amount * (transferFeePerc - tokenManager.getTokensInfo(tokenAddress).equilibriumFee))) /
  BASE_DIVISOR;

The incentive pool never really grows, breaking the economics of the entire protocol that relies on incentivizing low-liquidity pools with this pool.

Recommended Mitigation Steps

Fix the computation:

incentivePool[tokenAddress] +=
        (amount * (transferFeePerc - tokenManager.getTokensInfo(tokenAddress).equilibriumFee)) /
    BASE_DIVISOR;

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityProviders.sol#L273

Vulnerability details

Impact

Supported tokens can be turned off again by calling TokenManager.removeSupportedToken. Users won't be able to withdraw their liquidity anymore because of this check in removeLiquidity.

Recommended Mitigation Steps

Consider allowing withdrawals even if the token was unsupported to allow users to reclaim their funds.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L268

Vulnerability details

Impact

The executors can set their own tokenGasPrice arbitrarily high when calling sendFundsToUser, receiving huge gas rewards which are subtracted from the user's amount to receive. The user can lose all funds because of this. While the executors need to be fully trusted anyway, I still think this is an unnecessary additional trust assumption on the executors. The trust should be removed as much as possible.

Recommended Mitigation Steps

The owner should set a global tokenGasPrice that is used for gas refunds instead of executors choosing their own gas price.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityProviders.sol#L171

Vulnerability details

Impact

Owners can change the liquidityPool variable any time with the setLiquidityPool function. If a liquidity pool was already set and users added liquidity with addTokenLiquidity, the tokens are directly transferred to the liquidity pool and not kept in the LiquidityProviders contract. Changing the liquidityPool to a different contract will make it impossible for the users to withdraw their liquidity using removeLiquidity because the tokens are still in the old liquidityPool and cannot be retrieved.

All users will lose their funds.

Recommended Mitigation Steps

Changing the liquidityPool requires a sophisticated migration mechanism. Only allow setting the liquidityPool contract once.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityProviders.sol#L116

Vulnerability details

Impact

Owners can change the lpToken variable at any time with the setLpToken function. If an LP token was already set and users added liquidity with addTokenLiquidity and were minted a lpToken NFT, changing the lpToken to a different contract will make it impossible for the users to withdraw their liquidity using removeLiquidity.

All users will lose their funds.

Recommended Mitigation Steps

Changing the lpToken requires a sophisticated migration mechanism. Only allow setting the lpToken contract once.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L337

Vulnerability details

Impact

The gasPrice in getAmountToTransfer is in native tokens and it is subtracted from the user's requested token which is not the native token, is valued differently, and might even have different decimals.

// @audit in native tokens
uint256 totalGasUsed = initialGas - gasleft();
uint256 gasFee = totalGasUsed * tokenGasPrice;
// @audit amount and transferFeeAmount are in `tokenAddress`, cannot just subtract gasFee
amountToTransfer = amount - (transferFeeAmount + gasFee);

Receiving non-native tokens on a chain will almost always lead to reverts or losses for users because of this wrong amountToTransfer computation.

POC

• user wants to transfer 1000.0 USDC = 1e9 USDC as USDC has 6 decimals, from ETH to Polygon
• on the receiving POlygon chain, the executor calls sendFundsToUser(tokenAddress=USDC, amount=1e9, tokenGasPrice>0) and then amountToTransfer = 1e9 - gasFee will most likely underflow or take a huge cut on the user's USDC.

Recommended Mitigation Steps

The gasPrice cannot be a haircut on the amount as it is not always the native token.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityProviders.sol#L192

Vulnerability details

Impact

The public sharesToTokenAmount function does not check if the denominator totalSharesMinted[_tokenAddress] is zero. Neither do the callers of this function. The function will revert. Calling functions like getFeeAccumulatedOnNft and sharesToTokenAmount from another contract should never revert.

Recommended Mitigation Steps

Return 0 in case totalSharesMinted[_tokenAddress] is zero.

QA

• can frontrun all initialize functions
• Gas: addExecutors does not need onlyOwner modifier because addExecutor already has it
• Gas: removeExecutors does not need onlyOwner modifier because removeExecutor already has it
• Gas onlyValidLpToken function does not use the token return variable of the tokenMetadata call. The call can be removed
• Protocol does not support fee-on-transfer tokens, see addLiquidity and LiquidityPool.depositErc20. The amount value is stored and available to be withdrawn later but the contract receives amount - fees. Note that there are tokenChecks(tokenAddress) and the protocol would need to whitelist these non-standard tokens first.
• Users can lose native tokens because contracts have a receive() function. Check if it's really needed or consider restricting the callers to contracts that call it.
• Gas: getRewardAmount: The * 1e11 / 1e11 is not useful and does not give more precision. It can be removed
• Executors need to be trusted, otherwise, they can just call sendFundsToUser to transfer out any funds as they please. Consider adding threshold signatures to prevent all funds from being lost when a single executor is compromised.
• sendFundsToUser This balance check includes the not withdrawn fees: require(IERC20Upgradeable(tokenAddress).balanceOf(address(this)) >= amountToTransfer, "Not Enough Balance"). So in theory a transfer could steal the fees of LPs but it reverts earlier in getAmountToTransfer -> getTransferFee if that would be the case. The check should still use getCurrentLiquidity(tokenAddress) instead of the contract's balance.
• Gas: ifEnabled. Equivalent to shorter !areWhiteListRestrictionsEnabled || _cond
• Gas: Cache _getDigitsCount(fractionalPartTemp) in a variable instead of doing 2 calls.
• SVGs use off-chain class="..." attributes. The point of storing the SVGs on-chain is that they are fully self-contained and don't really on any off-chain parts to be able to reconstruct them. The CSS class is however not defined in the SVG and thus the image is determined by however the off-chain class is defined, defeating the purpose.