sendFundsToUser() does not verify that the user has deposited anything

[code423n4]

Lines of code

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

Vulnerability details

Impact

Users can request arbitrary amounts when requesting funds from the executor, because the deposit hash is not checked against actual deposits. The user can be the executor him/herself if they wish to rug-pull directly.

Proof of Concept

    function sendFundsToUser(
        address tokenAddress,
        uint256 amount,
        address payable receiver,
        bytes memory depositHash,
        uint256 tokenGasPrice,
        uint256 fromChainId
    ) external nonReentrant onlyExecutor tokenChecks(tokenAddress) whenNotPaused {
        uint256 initialGas = gasleft();
        require(
            tokenManager.getTransferConfig(tokenAddress).min <= amount &&
                tokenManager.getTransferConfig(tokenAddress).max >= amount,
            "Withdraw amnt not in Cap limits"
        );
        require(receiver != address(0), "Bad receiver address");

        (bytes32 hashSendTransaction, bool status) = checkHashStatus(tokenAddress, amount, receiver, depositHash);

        require(!status, "Already Processed");
        processedHash[hashSendTransaction] = true;

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

    function checkHashStatus(
        address tokenAddress,
        uint256 amount,
        address payable receiver,
        bytes memory depositHash
    ) public view returns (bytes32 hashSendTransaction, bool status) {
        hashSendTransaction = keccak256(abi.encode(tokenAddress, amount, receiver, keccak256(depositHash)));

        status = processedHash[hashSendTransaction];
    }

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

Even if the executor is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. See these examples where similar findings have been flagged as a high-severity issues.

Tools Used

Code inspection

Recommended Mitigation Steps

Store deposit hashes during deposit, and have checkHashStatus() require() that the hash exists

[tomarsachin2271]

Current implementation of executor nodes is centralised and being an executor on ExecutorManager is not open to everyone. Biconomy runs the executor nodes, so there's trust assumption on executor nodes that it won't call sendFundsToUser method without any deposit transaction on source chain.
Building a trustless cross chain messaging is in the roadmap though.

[pauliax]

#80 (comment)

[pauliax]

Grouping this together with a similar issue submitted by the same warden: #160