Biconomy Hyphen 2.0 contest - kyliek's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L352 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/TokenManager.sol#L101-L103

Vulnerability details

Impact

A liquidity provider's fund can be trapped when a token is no longer supported. There is no validation to check if there are any outstanding shares in LP tokens associated with the token.

Proof of Concept

A user minted LP tokens by adding liquidity when the _tokenAddress was still supported. When the user wants to removeLiquidity later on the token is no longer supported, thus unable to redeem the shares.

Recommended Mitigation Steps

When removing the support for a token, check for outstanding liquidity pool shares to make sure no users' fund is trapped.

    function removeSupportedToken(address tokenAddress) external tokenChecks(tokenAddress) onlyOwner {
        tokensInfo[tokenAddress].supportedToken = false;
    }

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L149-L173 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L263-L277

Vulnerability details

Impact

depositErc20 allows an attacker to specify the destination chain to be the same as the source chain and the receiver account to be the same as the caller account. This enables an attacker to drain the incentive pool without rebalancing the pool back to the equilibrium state.

Proof of Concept

This requires the attacker to have some collateral, to begin with. The profit also depends on how much the attacker has. Assume the attacker has enough assets.

In each chain, when the pool is very deficit (e.g. currentLiquidity is much less than providedLiquidity), which often mean there's a good amount in the Incentive pool after some high valued transfers, then do the following.

• step 1 : borrow the liquidityDifference amount such that one can get the whole incentivePool.

            uint256 liquidityDifference = providedLiquidity - currentLiquidity;
            if (amount >= liquidityDifference) {
                rewardAmount = incentivePool[tokenAddress];

• step 2 : call depositErc20() with toChainId being the same chain and receiver being msg.sender.

The executor will call sendFundsToUser to msg.sender. Then a rewardAmount, equivalent to the entire incentive pool (up to 10% of the total pool value), will be added to msg.sender minus equilibrium fee (~0.01%) and gas fee.

In the end, the pool is back to the deficit state as before, the incentive pool is drained and the exploiter pockets the difference of rewardAmount minus fees.

This attack can be repeated on each deployed chain multiple times whenever the incentive pool is profitable (particularly right after a big transfer).

Tools Used

Recommended Mitigation Steps

• Disallow toChainId to be the source chain by validating it in depositErc20 or in sendFundsToUser validate that fromChainId is not the same as current chain.

• require receiver is not msg.sender in depositErc20.

Lines of code

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

Vulnerability details

Impact

LP token holders can not redeem their tokens when the pool is in the deficit state, i.e. currentLiquidity << providedLiquidity. This is due to that LP shares are computed based on providedLiquidity and the actual available pool balance is based on currentLiquidity.

Proof of Concept

When a high valued withdrawal happens in the liquidity pool of the destination chain, the current liquidity will be reduced when the executor calls sendFundsToUser https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L285

and the pool contract balance will also be reduced by the same amount. The pool reached a deficit state with provided liquidity much bigger than current liquidity.

The LP shares are computed based on the value of totalReserve that is roughly equivalent to totalLiquidity + LpFees. In a deficit state, totalReserve could be much bigger than the available pool balance (up to 90% since max fee is 10%). If the LP token holder wants to redeem his shares,

        _decreaseCurrentLiquidity(_tokenAddress, _amount);

will underflow and revert and

        _transferFromLiquidityPool(_tokenAddress, _msgSender(), amountToWithdraw);

will revert because there is not enough balance.

Recommended Mitigation Steps

This is a tricky problem. On one hand, separating currentLiquidity from providedLiquidity made sure that by bridging tokens over, it will not inflate or deflate the pool. On the other hand, decoupling the two made it hard to compute the actual available liquidity to redeem LP shares. One may need to think through this a bit more.

Low

[L01] Outdated key dependencies

@openzeppelin/contracts                 4.3.0 -> 4.5.0 
@openzeppelin/contracts-upgradeable     4.3.0 -> 4.5.2  

[L02] Outdated compiler version

pragma solidity 0.8.0 is used throughout. The current version is 0.8.12. The recommended version is 0.8.11.

[L03] Same functions names for different return values

Both LiquidityPool.sol and liquidityProviders.sol have function of the name getCurrentLiquidty that gives two different values. This is very confusing. We recommend better naming for clarity.

See https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L132-L140

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

[L04] Different names referring to the same state variable

providedLiquidity, suppliedLiquidity and totalLiquidity are used to refer to the same state variable totalLiquidity for a token.

    mapping(address => uint256) public totalLiquidity; // Include Liquidity only

Please use one consistent name for the same variable.

See instances here.

• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L344
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L177
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L100-L102

[L05] Named return variable is not explicitly given

The function getRewardAmount gives back a named return variable rewardAmount. However when currentLiquidity >= providedLiquidity, there is no explicit return value.

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

[L06] function name transfer should be avoided

Function name transfer is used in multiple standards, particularly ERC721, which is implemented here for LP token. It is very confusing to have a custom transfer function in LiquidityPool.sol for some different purpose. This function is called only when LP shares are redeemed. We could use another name for clarity, e.g. redeemLPshares().

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

[L07] Use IERC20 instead of IERC20Upgradeable

Many existing candidate tokens are non-upgradeable ERC20 tokens. It is more appropriate just to use IERC20 instead of IERC20Upgradeable to avoid potential undiscovered upgradeability complexity.

            IERC20Upgradeable baseToken = IERC20Upgradeable(_tokenAddress);

Similarly one can use SafeERC20 instead of SafeERC20Upgradeable.

Some instances are

• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L170
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L293
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L379
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L405
• https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L273

Notes

[N01] Use UUPS proxy pattern instead of Transparent proxy pattern

This is recommended by OpenZeppelin.

The original proxies included in OpenZeppelin followed the Transparent Proxy Pattern. While this pattern is still provided, our recommendation is now shifting towards UUPS proxies, which are both lightweight and versatile.

see https://docs.openzeppelin.com/contracts/4.x/api/proxy#transparent-vs-uups

[N02] Shadowed state variables: _trustedForwarder

Argument _trustedForwarder in initialize in LiquidityPool.sol shadows the state variable of the same name in ERC2771ContextUpgradeable.

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

[N03] TokenManager is not upgradeable. Is it intentional?

[N04] Lack of NatSpec documentation pretty much for most functions and contracts.