[WP-H7] Infinite approval to an arbitrary address can be used to steal all the funds from the contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L131-L157

Vulnerability details

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L131-L157

function _startBridge(AnyswapData memory _anyswapData) internal {
    // Check chain id
    require(block.chainid != _anyswapData.toChainId, "Cannot bridge to the same network.");
    address underlyingToken = IAnyswapToken(_anyswapData.token).underlying();

    if (underlyingToken == IAnyswapRouter(_anyswapData.router).wNATIVE()) {
        IAnyswapRouter(_anyswapData.router).anySwapOutNative{ value: _anyswapData.amount }(
            _anyswapData.token,
            _anyswapData.recipient,
            _anyswapData.toChainId
        );
        return;
    }

    if (_anyswapData.token != address(0)) {
        // Has underlying token?
        if (underlyingToken != address(0)) {
            // Give Anyswap approval to bridge tokens
            LibAsset.approveERC20(IERC20(underlyingToken), _anyswapData.router, _anyswapData.amount);

            IAnyswapRouter(_anyswapData.router).anySwapOutUnderlying(
                _anyswapData.token,
                _anyswapData.recipient,
                _anyswapData.amount,
                _anyswapData.toChainId
            );
        } else {

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L59-L70

function approveERC20(
    IERC20 assetId,
    address spender,
    uint256 amount
) internal {
    if (isNativeAsset(address(assetId))) return;
    uint256 allowance = assetId.allowance(address(this), spender);
    if (allowance < amount) {
        if (allowance > 0) SafeERC20.safeApprove(IERC20(assetId), spender, 0);
        SafeERC20.safeApprove(IERC20(assetId), spender, MAX_INT);
    }
}

In the AnyswapFacet.sol, _anyswapData.router is from the caller's calldata, which can really be any contract, including a fake Anyswap router contract, as long as it complies to the interfaces used.

And in _startBridge, it will grant infinite approval for the _anyswapData.token to the _anyswapData.router.

This makes it possible for a attacker to steal all the funds from the contract.

Which we explained in [WP-H6], the diamond contract may be holding some funds for various of reasons.

PoC

Given:

• There are 100 USDC tokens in the contract.

1. The attacker can submit a startBridgeTokensViaAnyswap() with a FAKE _anyswapData.router.
2. Once the FAKE router contract deployed by the attacker got the infinite approval from the diamond contract, the attacker can call transferFrom() and take all the funds, including the 100 USDC in the contract anytime.

Recommendation

1. Whitelisting the _anyswapData.router rather than trusting user's inputs;
2. Or, only approve() for the amount that required for the current transaction instead of infinite approval.

[H3xept]

We are aware that the contract allows users to use latent funds, although we disagree on it being an issue as no funds (ERC20 or native) should ever lay in the contract. To make sure that no value is ever kept by the diamond, we now provide refunds for outstanding user value (after bridges/swaps).

[H3xept]

Duplicate of #130

[gzeoneth]

Warden highlighted the vulnerability in AnyswapFacet which would allow attacker to grant approval to arbitrary contract.

There can be fund leftover in the contract under normal operation, for example this tx. In fact, ~$300 worth of token is left in the LI.Fi smart contract on ETH mainnet 0x5a9fd7c39a6c488e715437d7b1f3c823d5596ed1 as of block 14597316. I don't think this is High Risk because the max amount lost is no more than allowed slippage, which can be loss to MEV too.