Biconomy Hyphen 2.0 contest - cccz's results

Lines of code

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

Vulnerability details

Impact

The sendFundsToUser function accepts the depositHash parameter, but does not perform any verification on the depositHash parameter, which allows the Executor to use the sendFundsToUser function arbitrarily to transfer tokens. This is a very serious centralization problem, if the private key of the Executor is leaked, the attacker will be able to consume all the tokens in the contract.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Validate the depositHash parameter in the sendFundsToUser function and disable the used depositHash.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L280-L310

Vulnerability details

Impact

In the LiquidityProviders contract, when the user provides liquidity, the _increaseLiquidity function will be called, and the user can call the removeLiquidity function to remove the liquidity. In addition, when the LiquidityPool contract's sendFundsToUser function is called, the LiquidityProviders contract's addLPFee function will be called. Consider the following situation:

1. User A calls the addTokenLiquidity function to provide 100 tokenA, and then User A gets 10010e18 shares. At this time, totalReserve[tokenA] is 100, and totalSharesMinted[tokenA] is 10010e18.

2. When other users deposit and withdraw tokenA in the LiquidityPool contract, the addLPFee function of the LiquidityProviders contract will be called, considering that the totalReserve[tokenA] has increased to 101.

3. User A calls the removeLiquidity function to take out 99 tokenA

lpFeeAccumulated = 100*101/100 - 100 = 1
lpSharesToBurn = (99*100/101+1*100/101)*10e18 = 99.0099 * 10e18

Since totalNFTShares - lpSharesToBurn < BASE_DIVISOR, so

lpSharesToBurn = 100*10e18
amountToWithdraw = 99+1 = 100

at this time

totalReserve[tokenA] = 101-100 =1
totalSharesMinted[tokenA] = 0

 (address _tokenAddress, uint256 nftSuppliedLiquidity, uint256 totalNFTShares) = lpToken.tokenMetadata(_nftId);
        require(_isSupportedToken(_tokenAddress), "ERR__TOKEN_NOT_SUPPORTED");

        require(_amount != 0, "ERR__INVALID_AMOUNT");
        require(nftSuppliedLiquidity >= _amount, "ERR__INSUFFICIENT_LIQUIDITY");
        whiteListPeriodManager.beforeLiquidityRemoval(_msgSender(), _tokenAddress, _amount);
        // Claculate how much shares represent input amount
        uint256 lpSharesForInputAmount = _amount * getTokenPriceInLPShares(_tokenAddress);

        // Calculate rewards accumulated
        uint256 eligibleLiquidity = sharesToTokenAmount(totalNFTShares, _tokenAddress);
        
        uint256 lpFeeAccumulated;

        // Handle edge cases where eligibleLiquidity is less than what was supplied by very small amount 
        if(nftSuppliedLiquidity > eligibleLiquidity) {
            lpFeeAccumulated = 0;
        } else {
            unchecked {
                lpFeeAccumulated = eligibleLiquidity - nftSuppliedLiquidity;
            }
        }
        // Calculate amount of lp shares that represent accumulated Fee
        uint256 lpSharesRepresentingFee = lpFeeAccumulated * getTokenPriceInLPShares(_tokenAddress);

        totalLPFees[_tokenAddress] -= lpFeeAccumulated;
        uint256 amountToWithdraw = _amount + lpFeeAccumulated;
        uint256 lpSharesToBurn = lpSharesForInputAmount + lpSharesRepresentingFee;

        // Handle round off errors to avoid dust lp token in contract
        if (totalNFTShares - lpSharesToBurn < BASE_DIVISOR) {
            lpSharesToBurn = totalNFTShares;
        }
        totalReserve[_tokenAddress] -= amountToWithdraw;
        totalLiquidity[_tokenAddress] -= _amount;
        totalSharesMinted[_tokenAddress] -= lpSharesToBurn;

4. User B calls the addTokenLiquidity function to provide liquidity for tokenA. Since totalReserve[tokenA]!=0, totalSharesMinted[tokenA] == 0, mintedSharesAmount is 0, require(mintedSharesAmount >= BASE_DIVISOR, "ERR__AMOUNT_BELOW_MIN_LIQUIDITY"); will fail.

    function _increaseLiquidity(uint256 _nftId, uint256 _amount) internal onlyValidLpToken(_nftId, _msgSender()) {
        (address token, uint256 totalSuppliedLiquidity, uint256 totalShares) = lpToken.tokenMetadata(_nftId);

        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");

Proof of Concept

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L280-L310 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L345-L395

Tools Used

None

Recommended Mitigation Steps

Consider adding handling for the case where totalSharesMinted[token] == 0 in the _increaseLiquidity function

    function _increaseLiquidity(uint256 _nftId, uint256 _amount) internal onlyValidLpToken(_nftId, _msgSender()) {
        (address token, uint256 totalSuppliedLiquidity, uint256 totalShares) = lpToken.tokenMetadata(_nftId);

        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 || totalSharesMinted[token] == 0) {
            mintedSharesAmount = BASE_DIVISOR * _amount;
        } else {
            mintedSharesAmount = (_amount * totalSharesMinted[token]) / totalReserve[token];
        }

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