[WP-H5] Refunds from failed bridging orders are not handled properly

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L161-L168

Vulnerability details

https://github.com/celer-network/sgn-v2-contracts/blob/b0cf02c15e25f66279420e3ff6a8b2fe07404bab/contracts/Bridge.sol#L53-L54

    * Must be greater than minimalMaxSlippage. Receiver is guaranteed to receive at least (100% - max slippage percentage) * amount or the
    * transfer can be refunded.
    */
function send(
    address _receiver,
    address _token,
    uint256 _amount,
    uint64 _dstChainId,
    uint64 _nonce,
    uint32 _maxSlippage // slippage * 1M, eg. 0.5% -> 5000
) external nonReentrant whenNotPaused {
    bytes32 transferId = _send(_receiver, _token, _amount, _dstChainId, _nonce, _maxSlippage);
    IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
    emit Send(transferId, msg.sender, _receiver, _token, _amount, _dstChainId, _nonce, _maxSlippage);
}

As per the documentation and comments on CBridge's code:

Must be greater than minimalMaxSlippage. Receiver is guaranteed to receive at least (100% - max slippage percentage) * amount or the transfer can be refunded.

For whatever reason that makes a bridging order end up refunded by the bridge operator, for example: cBridge, they will send the funds back to transfer.sender, which will be the diamond contract of li.fi.

https://github.com/celer-network/sgn-v2-contracts/blob/fcc40a30579c030c2a458c7e3b23cbc42295eedb/contracts/message/apps/BatchTransfer.sol#L90-L98

// called on source chain for handling of bridge failures (bad liquidity, bad slippage, etc...)
function executeMessageWithTransferRefund(
    address _token,
    uint256 _amount,
    bytes calldata _message
) external payable override onlyMessageBus returns (bool) {
    TransferRequest memory transfer = abi.decode((_message), (TransferRequest));
    IERC20(_token).safeTransfer(transfer.sender, _amount);
    return true;
}

In the current implementation, these refunds will just sit in the contract and the rightful owners of the funds wont have a way to claim their funds.

In essence, once the bridging order failed, the user will lose all their funds.

PoC

1. Alice sent a tx to bridge 100 USDC from Polygon to Fantom with cBridge;
2. cBridge refunded Alice's tx due to bridge failures (bad liquidity, bad slippage, etc...) and sent back the 100 USDC to the diamond contract.

There is no way for Alice to get back the 100 USDC.

And actual, the 100 USDC can be stolen by an attacker, see: [WP-H6], [WP-H7].

Recommendation

Consider adding a new method to refund users, in which you should check and set the refund status by the orderId and make sure one orderId cant be refunded more than once.

[H3xept]

If bridging fails in any facet other than cBridge the function will revert, returning all the tokens. In the case of cBridge, the refund is not automatically issued, so there's no risk of funds being stuck in the contract.

[gzeoneth]

Confirmed sponsor's claim regarding cBridge here.