LI.FI contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

Vulnerability details

Impact

https://github.com/code-423n4/2022-03-lifinance/blob/main/docs/GenericSwapFacet.md stated that _lifiData is strictly for analytics purposes. But _lifiData is used to set receivingAsset.

Proof of Concept

In GenericSwapFacet.swapTokensGeneric, _lifiData.receivingAssetId is used in LibAsset.getOwnBalance and LibAsset.transferAsset.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);
		...
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

In order to follow the policy, there should be a new parameter GenericSwapData.

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData, GenericSwapData _genericSwapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_genericSwapData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_genericSwapData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_genericSwapData.receivingAssetId, payable(msg.sender), postSwapBalance);
		...
    }

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L37 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L74 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L80 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L131 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L136 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L137 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L149 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L151 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L159 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L161

Vulnerability details

Impact

AnyswapFacet does not check whether the passed _anyswapData.router is benign. This allows attackers to provide malicious routers to "exchange" tokens and native asset in an extremely favorable rate from Diamond, or potentially stealing all remaining tokens when chained with the LibAsset.approveERC20 bug discussed in a separate report.

Proof of Concept

Opposed to other contracts, AnyswapFacet does not have a fixed router contract explicitly assigned by Diamond owner. Instead, it allows users to pass the desired router along with other function arguments. This opens up a chance for attackers to craft a fake _anyswapData.router.

To demonstrate the exploit, we will focus on the startBridgeTokensViaAnyswap function and discuss the case of profiting by exchanging USDT to ether.

Below is the code covered in our attack. Notice while we focus our attack on startBridgeTokensViaAnyswap here, swapAndStartBridgeTokensViaAnyswap is also suscepetible to the same attack logic.

    function startBridgeTokensViaAnyswap(LiFiData memory _lifiData, AnyswapData calldata _anyswapData) public payable {
        address underlyingToken = IAnyswapToken(_anyswapData.token).underlying();
        if (_anyswapData.token != address(0) && underlyingToken != IAnyswapRouter(_anyswapData.router).wNATIVE()) {
            if (underlyingToken == address(0)) {
                underlyingToken = _anyswapData.token;
            }

            uint256 _fromTokenBalance = LibAsset.getOwnBalance(underlyingToken);
            LibAsset.transferFromERC20(underlyingToken, msg.sender, address(this), _anyswapData.amount);

            require(
                LibAsset.getOwnBalance(underlyingToken) - _fromTokenBalance == _anyswapData.amount,
                "ERR_INVALID_AMOUNT"
            );
        } else {
            require(msg.value == _anyswapData.amount, "ERR_INVALID_AMOUNT");
        }

        _startBridge(_anyswapData);

        ...
    }

    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 {
                // Give Anyswap approval to bridge tokens
                LibAsset.approveERC20(IERC20(_anyswapData.token), _anyswapData.router, _anyswapData.amount);

                IAnyswapRouter(_anyswapData.router).anySwapOut(
                    _anyswapData.token,
                    _anyswapData.recipient,
                    _anyswapData.amount,
                    _anyswapData.toChainId
                );
            }
        }
    }

Imagine a scenario where there are currently some ether in the Diamond contract (or we can trigger come bridges to send ether to the contract either due to timeout/expiry or by simply fulfilling a swap). Our target is to siphon those ether by depositing USDT on a 1:1 basis.

Our argument setup is approximately like this, we choose a _anyswapData.token that has USDT as IAnyswapToken(_anyswapData.token).underlying(), then design a malicious _anyswapData.router that has IAnyswapRouter(_anyswapData.router).wNATIVE() return address(0) on first call and address(USDT) on second call. We also set _anyswapData.amount to be equal to current address(Diamond).balance.

Now we can trace what will happen throughout the process.

1. underlyingToken will be set to USDT
2. since underlyingToken != wNATIVE (address(USDT) != address(0)), _anyswapData.amount of USDT will be transferred from caller(attacker/us) to Diamond
3. pass checks and proceed to enter _startBridge
4. The second call of wNATIVE returns address(USDT), so now underlyingToken == wNATIVE
5. IAnyswapRouter(_anyswapData.router).anySwapOutNative{ value: _anyswapData.amount }(...) is called, we now can walk away with _anyswapData.amount of ether

The exploit is pretty simple, but we still need to deal with how to have the fake router return different wNATIVE for the first and second call. If we assume that the call is done with call opcode, it is pretty easy to just keep an internal storage in the fake router contract to decide on what value to return. However, if staticcall is used, we can no longer change storage value within the fake router. Thankfully under this case, it is still possible to utilize remaining gas to decide when to switch from returning address(0) to address(USDT).

The attack can also be adapted to stealing tokens by depositing ether, under cases where valuable tokens such as WBTC are held by the Diamond contract. In fact, when used together with the LibAsset.approveERC20 function which approves UINT256_MAX (instead of amount specified in argument) to the spender, it is possible specify a 0 _anyswapData.amount and proceed to transfer all tokens from Diamond to attacker.

Overall, this lack of check on router fully compromises any asset stored in the Diamond contract.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

While one of the strengths of anySwap is its flexibility, we have shown in this context that flexibility equates vulnerability. The easiest way to fix this is to adopt a whitelist where Diamond contract owners may add/remove trusted routers.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L12 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L33

Vulnerability details

Impact

LibSwap.swap only transfers non-native tokens from msg.sender when balance of said token is lesser than requested fromAmont, thus if there are any remaining tokens in the contract, attacker can easily utilize those tokens for swapping and in turn claim ownership of those

Proof of Concept

The following exploit is demonstrated using GenericSwapFacet. But the vulnerability can be triggered from almost every faucet that uses _executeSwaps.

• Assume that the WETH balance of the contract is 100 WETH
• Alice call GenericSwapFacet.swapTokensGeneric and _lifiData.receivingAssetId is set to native asset (can be set to any asset except WETH)

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);

        ...
    }

• Alice uses WETH as sendingAsset. And the fromAmount is 100. Since LibAsset.getOwnBalance(fromAssetId) < fromAmount, no WETH will be collected from Alice
• The contract swap out 100 WETH to DEX, and swap in some native assets. Then it will send the native assets received DEX to Alice.
• Obviously, Alice paid nothing(or only gas) to get the assets.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42

    function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        uint256 fromAmount = _swapData.fromAmount;
        uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
        address fromAssetId = _swapData.sendingAssetId;
        if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
            LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
        }

        if (!LibAsset.isNativeAsset(fromAssetId)) {
            LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
        }

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
        if (!success) {
            string memory reason = LibUtil.getRevertMsg(res);
            revert(reason);
        }

        ...
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

Before doing _executeSwaps. the contract should track the preBalance.

And use

prebalance - LibAsset.getOwnBalance(fromAssetId) < fromAmount

instead of

LibAsset.getOwnBalance(fromAssetId) < fromAmount

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L12 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42

Vulnerability details

Impact

_executeSwaps is always wrapped in a loop, however, it uses msg.value to decide the amount of native tokens to use without any additional checks. Attackers can easily drain the existing funds by repeatedly swapping native asset.

Proof of Concept

The following exploit is demonstrated using GenericSwapFacet. But the vulnerability can be triggered from every faucet that uses _executeSwaps.

• Assume that the native asset balance of the contract is 100eth
• Alice calls GenericSwapFacet.swapTokensGeneric with msg.value = 100eth

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);

        ...
    }

• The _swapData array sent by Alice has two SwapData. So it calls LibSwap.swap twice.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L12

    function _executeSwaps(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) internal {
        // Swap
        for (uint8 i; i < _swapData.length; i++) {
            require(
                ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,
                "Contract call not allowed!"
            );

            LibSwap.swap(_lifiData.transactionId, _swapData[i]);
        }
    }

• Alice uses native asset as sendingAsset in both swap data. So LibSwap.swap won't transfer any more native asset from Alice.
• _swapData.callTo.call{ value: msg.value }(_swapData.callData) is executed twice. The contract actually send 200 eth to DEX. But only receive 100 eth from Alice.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42

    function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        uint256 fromAmount = _swapData.fromAmount;
        uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
        address fromAssetId = _swapData.sendingAssetId;
        if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
            LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
        }

        if (!LibAsset.isNativeAsset(fromAssetId)) {
            LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
        }

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
        if (!success) {
            string memory reason = LibUtil.getRevertMsg(res);
            revert(reason);
        }

        …
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

When sending native asset to DEX in LibSwap.swap, the contract should do bookkeeping on the actual used native asset and received native asset. A common way to do this is to extract msg.value into a local variable, and subtract from the said variable every time when native asset is used. Checks should also be enforced against this local variable instead of the raw msg.value.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L150

Vulnerability details

Impact

When sendNative() is called, cbridge expects _amount of native tokens to be sent to it. However, CBridgeFacet does not send any native asset, resulting in a certain revert.

Proof of Concept

In CBridgeFacet._startBridge, ICBridge(bridge).sendNative() doesn't have value set. Thus no native asset will be sent to the bridge.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L150

    function _startBridge(CBridgeData memory _cBridgeData) internal {
        Storage storage s = getStorage();
        address bridge = _bridge();

        // Do CBridge stuff
        require(s.cBridgeChainId != _cBridgeData.dstChainId, "Cannot bridge to the same network.");

        if (LibAsset.isNativeAsset(_cBridgeData.token)) {
            ICBridge(bridge).sendNative(
                _cBridgeData.receiver,
                _cBridgeData.amount,
                _cBridgeData.dstChainId,
                _cBridgeData.nonce,
                _cBridgeData.maxSlippage
            );
        } else {
            // Give CBridge approval to bridge tokens
            LibAsset.approveERC20(IERC20(_cBridgeData.token), bridge, _cBridgeData.amount);
            // solhint-disable check-send-result
            ICBridge(bridge).send(
                _cBridgeData.receiver,
                _cBridgeData.token,
                _cBridgeData.amount,
                _cBridgeData.dstChainId,
                _cBridgeData.nonce,
                _cBridgeData.maxSlippage
            );
        }
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

Set value

            ICBridge(bridge).sendNative{ value: msg.value }(
                _cBridgeData.receiver,
                _cBridgeData.amount,
                _cBridgeData.dstChainId,
                _cBridgeData.nonce,
                _cBridgeData.maxSlippage
            );

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L57 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L92 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L159

Vulnerability details

Impact

A malicious _cBridgeData.token can be crafted to re-enter facet. While no immediate attack vectors are found, preventive measures are suggested to fully close off such attack chances.

Proof of Concept

In _startBridge, LibAsset.approveERC20(IERC20(_cBridgeData.token), ...) is called on potentially malicious _cBridgeData.token. Since approve will be called with call opcode, it is possible to utilize this chance to re-enter facet.

We haven't identified any potential attack vectors except withdrawing previously transferred _cBridgeData.token, which is pretty much next to useless considering we already have full control over _cBridgeData.token. But generally, reducing the attack surface, especially for something as dangerous as reentrancy is a good thing to do.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Validate _cBridgeData.token if possible. If this is not desired due to reduced flexibility of facet, add nonReentrant modifier to public/external functions instead.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L38 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L68

Vulnerability details

Impact

In LibAsset.approveERC20, It approves MAX_INT instead of amount. It allows any potentially malicious/exploited bridge/dex to wreck havock and steal all said token instead of limiting attacks to only the explicitly approved amount. Attacks due to inappropriate approval have happened before, and there is no reason to believe that it is impossible to happen in the future.

Proof of Concept

• In LibSwap.swap, it seems like LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount) should approve fromAmount.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L38

    function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        uint256 fromAmount = _swapData.fromAmount;
        uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
        address fromAssetId = _swapData.sendingAssetId;
        if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
            LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
        }

        if (!LibAsset.isNativeAsset(fromAssetId)) {
            LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
        }
		...
	}

• However, LibAsset.approveERC20 approves MAX_INT.

    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);
        }
    }

• fromAmount is not used in _swapData.callTo.call and the contract approves MAX_INT. Thus it can transfer more assets than fromAmount.

    function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        ...
        if (!LibAsset.isNativeAsset(fromAssetId)) {
            LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
        }
		...
		(bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
		...
	}

Tools Used

Manual code review.

Recommended Mitigation Steps

Use

SafeERC20.safeApprove(IERC20(assetId), spender, amount);

instead of

SafeERC20.safeApprove(IERC20(assetId), spender, MAX_INT);

Summary

We list 5 low-critical findings and 1 non-critical finding here:

• (LOW) AnyswapFacet implicitly rejects address(0) as _anyswapData.token and reverts without warning
• (LOW) CBridgeFacet allows more than _cBridgeData.amount of native assets to be sent to it
• (LOW) Anyswap router version confusion
• (LOW) NXTPFacet does not check whether _nxtpData.invariantData.sendingChainId equals current chainId
• (LOW) NXTPFacet does not check _nxtpData.invariantData.callTo and _nxtpData.invariantData.callDataHash equals current chainid
• (Non-Critical) initializeDiamondCut implementation disagrees with EIP-2535

In summary of recommended security practices, it's better to check native assets, chainId and 3rd party bridge versions to avoid abuse. Also, it’s better to follow EIP-2535 proposal and check the exact amount for better practices.

(LOW) AnyswapFacet implicitly rejects address(0) as _anyswapData.token and reverts without warning

Impact

AnyswapFacet has a different interface for accepting native assets compared to other facets. This might lead to confusion and misuse of the facet, and result in loss of gas for contract users.

The logic of deciding which token to use in Anyswap is more complex than other facets. Overall, the decision is done twice, first for checking (and transferring funds) from the user, and then for deciding which Anyswap router function to call.

logic for startBridgeTokensViaAnyswap

1. if _anyswapData.token == address(0), native asset is used
2. if _anyswapData.token.underlying() == _anyswapData.router.wNATIVE(), native asset is used
3. if _anyswapData.token.underlying() == address(0), _anyswapData.token is used
4. if _anyswapData.token.underlying() is used

logic for _startBridge

1. if _anyswap.token.underlying() == _anyswapData.router.wNATIVE(), call anySwapOutNative()
2. if _anyswapData.token == address(0), do nothing (note that this path would revert upon calling IAnyswapToken(_anyswapData.token).underlying() in _startBridge
3. if _anyswapData.token.underlying() == address(0), call anySwapOut()
4. call anySwapOutUnderlying()

The main mismatch rises when _anyswapData.token == address(0), startBridgeTokensViaAnyswap accepts those cases and uses native asset, while _startBridge does not handle this and implicitly reverts.

Since other facets usually use address(0) as native asset, we judged that users might be confused by the behaviour of AnyswapFacet, especially since it does not reject address(0) early in call, and only reverts at a relatively late stage.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L37 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L49 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L74 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L80 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L95 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L131 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L136 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L145

Recommended Mitigation Steps

It would be more appropriate to follow the same pattern as other facets, and add code to handle address(0) in _startBridge. Or at the very least, disallow address(0) early in startBridgeTokensViaAnyswap and swapAndStartBridgeTokensViaAnyswap and make it explicit that address(0) as token is not allowed.

(LOW) CBridgeFacet allows more than _cBridgeData.amount of native assets to be sent to it

Impact

Allowing excessive native assets to be sent is unnecessary, and would potentially lead to loss of the caller's asset.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L68

Recommended Mitigation Steps

Use == rather than >=

require(msg.value == _cBridgeData.amount, "ERR_INVALID_AMOUNT");

(LOW) Anyswap router version confusion

Impact

Anyswap router used in deployment/test is not compatible with AnyswapFacet function calls.

The provided Anyswap router address in test/facet/AnyswapFacet.test.ts and config/anyswap.ts are both implementing AnyswapV4Router. However, anySwapOutNative is only implemented in later versions of Anyswap router (e.g. AnyswapV6Router). While this is not strictly included in audit scope, we deem it appropriate to bring attention to the mismatch, in case of incorrect deployment in the future.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L137 https://github.com/code-423n4/2022-03-lifinance/blob/main/config/anyswap.ts#L14 https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/AnyswapFacet.test.ts#L14

Recommended Mitigation Steps

Use a newer Anyswap router address in deployment and test script instead.

(LOW) NXTPFacet does not check whether _nxtpData.invariantData.sendingChainId equals current chainId

Impact

Diamond might be abused to serve as router and call prepare on the receiving chain.

In the nxtp protocol, prepare is called by the sender in the sending chain and by router in the receiving chain. Diamond only intended to serve as the sender, but since it does not check _nxtpData.invariantData.sendingChainId against the current chainId, it is possible to abuse the field and call prepare as a router instead.

While the above abuse is theoretically possible, there are several checks in the nxtp protocol that must be satisfied in order to perform such actions. Including getting Diamond approved as a valid router, making bids in place of Diamond and so on. Overall, the trick is quite difficult to pull off.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L185

Recommended Mitigation Steps

Validate _nxtpData.invariantData.sendingChainId against chainId.

(LOW) NXTPFacet does not check _nxtpData.invariantData.callTo and _nxtpData.invariantData.callDataHash equals current chainid

Impact

Intended fulfill call back can be skipped or replaced with other functions.

The design of the NXTPFacet is that the two fulfill functions will be called on the receiving chain. However, there are no checks against whether the two helper functions completeBridgeTokensViaNXTP and swapAndCompleteBridgeTokensViaNXTP are passed into as calldata, and whether Diamond is even assigned as callTo. This allows users to assign arbitrary callbacks on the receiving chain.

While this does not pose any direct threats, it would be appropriate to check the two calling fields and conform to contract usage design.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L185

Recommended Mitigation Steps

Validate _nxtpData.invariantData.callTo and _nxtpData.invariantData.callDataHash against the intended callback functions.

(Non-Critical) initializeDiamondCut implementation disagrees with EIP-2535

Impact

The original proposal of Diamond specifies that _calldata is allowed to carry additional information even when _init is address(0). Current implementation disagrees with the proposal.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L185

Recommended Mitigation Steps

Refer to EIP-2535: https://eips.ethereum.org/EIPS/eip-2535

If the _init value is address(0) then _calldata execution is skipped. In this case _calldata can contain 0 bytes or custom information.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L67 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L92 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L110 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L125 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L33 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L52 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L65 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L70 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L48 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondLoupeFacet.sol#L24

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}