[WP-H22] Attacker can deposit from and to the same chainId to steal from the incentivePool

[code423n4]

Lines of code

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

Vulnerability details

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

function depositErc20(
    uint256 toChainId,
    address tokenAddress,
    address receiver,
    uint256 amount,
    string memory tag
) public tokenChecks(tokenAddress) whenNotPaused nonReentrant {
    require(
        tokenManager.getDepositConfig(toChainId, tokenAddress).min <= amount &&
            tokenManager.getDepositConfig(toChainId, tokenAddress).max >= amount,
        "Deposit amount not in Cap limit"
    );
    require(receiver != address(0), "Receiver address cannot be 0");
    require(amount != 0, "Amount cannot be 0");
    address sender = _msgSender();

    uint256 rewardAmount = getRewardAmount(amount, tokenAddress);
    if (rewardAmount != 0) {
        incentivePool[tokenAddress] = incentivePool[tokenAddress] - rewardAmount;
    }
    liquidityProviders.increaseCurrentLiquidity(tokenAddress, amount);
    SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(tokenAddress), sender, address(this), amount);
    // Emit (amount + reward amount) in event
    emit Deposit(sender, tokenAddress, receiver, toChainId, amount + rewardAmount, rewardAmount, tag);
}

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

function depositNative(
    address receiver,
    uint256 toChainId,
    string memory tag
) external payable whenNotPaused nonReentrant {
    require(
        tokenManager.getDepositConfig(toChainId, NATIVE).min <= msg.value &&
            tokenManager.getDepositConfig(toChainId, NATIVE).max >= msg.value,
        "Deposit amount not in Cap limit"
    );
    require(receiver != address(0), "Receiver address cannot be 0");
    require(msg.value != 0, "Amount cannot be 0");

    uint256 rewardAmount = getRewardAmount(msg.value, NATIVE);
    if (rewardAmount != 0) {
        incentivePool[NATIVE] = incentivePool[NATIVE] - rewardAmount;
    }
    liquidityProviders.increaseCurrentLiquidity(NATIVE, msg.value);
    emit Deposit(_msgSender(), NATIVE, receiver, toChainId, msg.value + rewardAmount, rewardAmount, tag);
}

In the current implementation, users are allowed to set toChainId when depositErc20() or depositNative(), when there are some incentive rewards, the user can get more tokens on toChainId.

However, there is no check that prevents the toChainId to be the current chainId, so that when the incentive rewards are high enough, an attacker or a malicious user can set the toChainId to the current chainId and steal the incentive rewards without helping the protocol to liquidity to the current chain, instead, it actually lowers the liquidity.

PoC

Given:

• Balance of contract on Mainnet = 10 ETH
• incentivePool[NATIVE] = 1 ETH
• rewardAmount for 1 ETH is 0.01 ETH

1. Alice depositNative() on Mainnet with amount = 1 ETH, toChainId = 1, receiver = Alice

• event Deposit(alice, NATIVE, alice, 1, 1.01 ETH, 0.01 ETH, tag) emitted
• Balance of contract on Mainnet = 11 ETH
• Alice paid 1 ETH

2. Executor call sendFundsToUser() on Mainnet, amount = 1.01 ETH

• Balance of contract on Mainnet = 9.99 ETH
• Alice got 1.01 ETH back

Recommendation

Consider adding checks to require toChainId != current chainId.

[ankurdubey521]

Duplicate of #87

[pauliax]

#87