LI.FI contest - kirk-baird's results

Lines of code

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

Vulnerability details

Impact

When calling Swapper._executeSwaps() there are no checks to ensure the received token matches the final swapped token. If these are different it may result in user funds being locked in the contract.

This issue is present in each of the following functions:

• GenericSwapFacet.swapTokensGeneric()
• HopFacet.swapAndStartBridgeTokensViaHop()
• AnyswapFacet.swapAndStartBridgeTokensViaAnyswap()
• CBridgeFacet.swapAndStartBridgeTokensViaCBridge()
• NXTPFacet.swapAndCompleteBridgeTokensViaNXTP()
• NXTPFacet.swapAndStartBridgeTokensViaNXTP()

Proof of Concept

For example consider GenericSwapFacet.swapTokensGeneric(). When calling _executeSwaps() the final swapped will be _swapData[_swapData.length - 1].receivingAssetId there is no gaurantee this matches _lifiData.receivingAssetId.

Since _lifiData.receivingAssetId is the asset that is measured and transferred to the user any discrepancies between these two will result in user funds being lost to the protocol. That is because they will need to transfer funds in to perform the swap but will not receive the funds out.

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

Recommended Mitigation Steps

Consider adding the checks

• GenericSwapFacet.swapTokensGeneric() => require(_lifiData.receivingAssetId == _swapData[_swapData.length - 1].receivingAssetId) in .
• HopFacet.swapAndStartBridgeTokensViaHop() => require(sendingAssetId == _swapData[_swapData.length - 1].receivingAssetId)
• AnyswapFacet.swapAndStartBridgeTokensViaAnyswap() => Depending on if it is ERC20 or native tokens
  • require(underlyingToken == _swapData[_swapData.length - 1].receivingAssetId)
  • require(LibAsset.NATIVE_ASSETID == _swapData[_swapData.length - 1].receivingAssetId)
• CBridgeFacet.swapAndStartBridgeTokensViaCBridge() => Depending on if it is ERC20 or native tokens
  • require(_cBridgeData.token == _swapData[_swapData.length - 1].receivingAssetId)
  • require(LibAsset.NATIVE_ASSETID == _swapData[_swapData.length - 1].receivingAssetId)
• NXTPFacet.swapAndCompleteBridgeTokensViaNXTP() = > require(finalAssetId == _swapData[_swapData.length - 1].receivingAssetId)
• NXTPFacet.swapAndStartBridgeTokensViaNXTP() = > require(sendingAssetId == _swapData[_swapData.length - 1].receivingAssetId)

Note it's also worth checking _swapData.length > 0.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35-L53

Vulnerability details

Impact

In AnyswapFacet.sol we parse arbitrary data in _anyswapData allowing an attacker to drain funds (ERC20 or native tokens) from the LiFi contract.

Functions effected:

• AnyswapFacet.startBridgeTokensViaAnyswap()
• AnyswapFacet.swapAndStartBridgeTokensViaAnyswap()

Proof of Concept

This attack works in AnyswapFacet.startBridgeTokensViaAnyswap() by having a malicious _anyswapData.token which may change the value return in IAnyswapToken(_anyswapData.token).underlying();.

First we have the first call to IAnyswapToken(_anyswapData.token).underlying(); return a malicious ERC20 contract in the attackers control. This allows for transferring these malicious ERC20 tokens to pass the required balance checks.

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

            require(
                LibAsset.getOwnBalance(underlyingToken) - _fromTokenBalance == _anyswapData.amount,
                "ERR_INVALID_AMOUNT"
            );

The function will then call _startBridge() which again does address underlyingToken = IAnyswapToken(_anyswapData.token).underlying(); we have the malicious _anyswapData.token return a different address, one which the LiFi contract has balance for (a native token or ERC20).

We will therefore execute the following which will either approve or transfer funds to _anyswapData.router for a different underlyingtoken to the one which supplied the funds to LiFi.

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

Since _anyswapData.router is an address in the attackers control they either are transferred native tokens or they have an allowance of ERC20 tokens that they can spend arbitrarily.

The attack is almost identical in swapAndStartBridgeTokensViaAnyswap()

Recommended Mitigation Steps

Consider whitelisting both Anyswap tokens and Anyswap routers (using two distinct whitelists) restricting the attackers ability to use malicious contracts for this attack.

Consider also only calling IAnyswapToken(_anyswapData.token).underlying() once and passing this value to _startBridge().

Lines of code

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

Vulnerability details

Impact

The function will transfer tokens from msg.sender if and only if the current balance of the token is less than the fromAmount for a swap as seen in the lines below.

        if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
            LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
        }

This poses two potential threats. The first is if a genuine user is making a swap with multiple paths and has incorrectly calculated fromAmount for the second swap (e.g. if the price has changed). If LibAsset.getOwnBalance(fromAssetId) < fromAmount where LibAsset.getOwnBalance(fromAssetId) should be the output of the previous swap (though it includes residual balance). Then the user will be charged the entire fromAmount which may revert if the user does not have sufficient funds or has not approved the LiFi contract. The user should not be charged then entire fromAmount since they have already contributed LibAsset.getOwnBalance(fromAssetId) from the previous swap. So even if a user was just one wei short due (possibly due to a price decrease) they will be forced to transfer the entire fromAmount for the next swap step.

The second issue is if there is any balance in the LiFi protocol the user may call LibSwap.swap() with fromAmount = LibAsset.getOwnBalance(fromAssetId) as a result transferFromERC20() will not be called. The function will continue to swap the asset and the user will receive the swapped tokens. Therefore, the user has not paid anything yet swapped the current balance of a ERC20 to in LiFi contract for another ERC20 token to which the attacker will receive.

Proof of Concept

Call GenericSwapToken.swapTokensGeneric() with fromAmount = LibAsset.getOwnBalance(fromAssetId) pick any output token and this will be transferred to the attacker.

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

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

        emit LiFiTransferStarted(
            _lifiData.transactionId,
            _lifiData.integrator,
            _lifiData.referrer,
            _lifiData.sendingAssetId,
            _lifiData.receivingAssetId,
            _lifiData.receiver,
            _lifiData.amount,
            _lifiData.destinationChainId,
            block.timestamp
        );
    }

Recommended Mitigation Steps

To mitigate this issue consider passing a boolean variable as a function parameter in swap() this variable will indicate if it is the first step (i.e. first external call) in the swap path. If it is the first step require fromAmount to be transferred from the msg.sender or require fromAmount == msg.value for the native token.

The following steps must use the toAmount from the previous step. To do this modify swap() to return toAmount and have Swapper._executeSwap() pass previousToAmount as a function parameter to LibSwap.swap().

Lines of code

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

Vulnerability details

Impact

msg.value is attached multiple times to external swap calls in LibSwap.swap().

If Swapper._executeSwaps() is called with the native token as the swapData.fromAssetId more than once and msg.value > 0 then more value will be transferred out of the contract than is received since msg.value will be transferred out _swapData.length times.

The impact is that the contract can have all the native token balance drained by an attacker who has makes repeated swap calls from the native token into any other ERC20 token. Each time the original msg.value of the sender will be swapped out of the contract. This attack essentially gives the attacker _swapData.length * msg.value worth of native tokens (swapped into another ERC20) when they should only get msg.value.

Proof of Concept

Swapper._executeSwaps() iterates over a list of SwapData calling LibSwap.swap() each time (note this is an internal call).

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

Inside LibSwap.swap() we make an external call to _swapData.callTo with value : msg.value. Due to the loop in Swapper._executeSwaps() this repeatedly sends the original msg.value in the external call.

        (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);

Recommended Mitigation Steps

This issue may be mitigated by only allowing fromAssetId to be the native token once in _swapData in Swapper._executeSwaps(). If it occurs more than once the transaction should revert.

Lines of code

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

Vulnerability details

Impact

msg.value is attached to external swap calls in LibSwap.swap() even if the fromAssetId is not the native token.

The function LibSwap.swap() will always include msg.value as seen on the line _swapData.callTo.call{ value: msg.value }(_swapData.callData);. If the sender has sent msg.value in the transaction, for example if they have multiple swap paths and the first one is the native token. Then this value is included for all other swap paths.

The impact is that there is either insufficient balance in the contract in which case the transaction will always revert. Alternatively, if there is sufficient balance in the contract then it will be transferred in the external call potentially draining the contract of funds.

Proof of Concept

In the function below msg.value is always sent in the external call even if the fromAssetId is not the native token.

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

Recommended Mitigation Steps

Consider only attaching value to the external call if fromAssetId is the native token. Alternatively do not allow for the native token to be used in the protocol and instead enforce it to be swapped in an ERC20.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondCutFacet.sol#L14-L22 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L92-L121 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L74-L110 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L85-L102 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L150-L171

Vulnerability details

Impact

There is a reenterancy vulnerability in functions which call Swapper._executeSwap() which would allow the attacker to change their postSwapBalance.

The functions following similar logic to that seen in GenericSwapFacet.swapTokensGeneric().

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

This logic records the balance before and after the _executeSwaps() function. The difference is then transferred to the msg.sender.

The issue occurs since it is possible for an attacker to reenter this function during _executeSwaps(), that is because execute swap makes numerous external calls, such as to the AMM, or to untrusted ERC20 token addresses.

If a function is called such as WithdrawFacet.withdraw() this will impact the calculations of postSwapBalance which will account for the funds transferred out during withdrawal. Furthermore, any functions which transfers funds into the contract will also be counted in the postSwapBalance calculations.

Vulnerable Functions:

• GenericSwapFacet.swapTokensGeneric()
• CBridgeFacet.swapAndStartBridgeTokensViaCBridge()
• AnyswapFacet.swapAndStartBridgeTokensViaAnyswap()
• HopFacet.swapAndStartBridgeTokensViaHop()
• NXTPFacet.swapAndStartBridgeTokensViaNXTP()
• NXTPFacet.swapAndCompleteBridgeTokensViaNXTP()

Proof of Concept

GenericSwapFacet.swapTokensGeneric()

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

CBridgeFacet.swapAndStartBridgeTokensViaCBridge()

    function swapAndStartBridgeTokensViaCBridge(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        CBridgeData memory _cBridgeData
    ) public payable {
        if (_cBridgeData.token != address(0)) {
            uint256 _fromTokenBalance = LibAsset.getOwnBalance(_cBridgeData.token);

            // Swap
            _executeSwaps(_lifiData, _swapData);

            uint256 _postSwapBalance = LibAsset.getOwnBalance(_cBridgeData.token) - _fromTokenBalance;

            require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

            _cBridgeData.amount = _postSwapBalance;
        } else {
            uint256 _fromBalance = address(this).balance;

            // Swap
            _executeSwaps(_lifiData, _swapData);

            uint256 _postSwapBalance = address(this).balance - _fromBalance;

            require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

            _cBridgeData.amount = _postSwapBalance;
        }

        _startBridge(_cBridgeData);

AnyswapFacet.swapAndStartBridgeTokensViaAnyswap()

    function swapAndStartBridgeTokensViaAnyswap(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        AnyswapData memory _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);

            // Swap
            _executeSwaps(_lifiData, _swapData);

            uint256 _postSwapBalance = LibAsset.getOwnBalance(underlyingToken) - _fromTokenBalance;

            require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

            _anyswapData.amount = _postSwapBalance;
        } else {
            uint256 _fromBalance = address(this).balance;

            // Swap
            _executeSwaps(_lifiData, _swapData);

            require(address(this).balance - _fromBalance >= _anyswapData.amount, "ERR_INVALID_AMOUNT");

            uint256 _postSwapBalance = address(this).balance - _fromBalance;

            require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

            _anyswapData.amount = _postSwapBalance;
        }

        _startBridge(_anyswapData);

HopFacet.swapAndStartBridgeTokensViaHop()

    function swapAndStartBridgeTokensViaHop(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        HopData memory _hopData
    ) public payable {
        address sendingAssetId = _bridge(_hopData.asset).token;

        uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 _postSwapBalance = LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance;

        require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

        _hopData.amount = _postSwapBalance;

        _startBridge(_hopData);

NXTPFacet.swapAndStartBridgeTokensViaNXTP()

    function swapAndStartBridgeTokensViaNXTP(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        ITransactionManager.PrepareArgs memory _nxtpData
    ) public payable {
        address sendingAssetId = _nxtpData.invariantData.sendingAssetId;
        uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 _postSwapBalance = LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance;

        require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

        _nxtpData.amount = _postSwapBalance;

        _startBridge(_lifiData.transactionId, _nxtpData);

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP()

    function swapAndCompleteBridgeTokensViaNXTP(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        address finalAssetId,
        address receiver
    ) public payable {
        uint256 startingBalance = LibAsset.getOwnBalance(finalAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(finalAssetId);

        uint256 finalBalance;

        if (postSwapBalance > startingBalance) {
            finalBalance = postSwapBalance - startingBalance;
            LibAsset.transferAsset(finalAssetId, payable(receiver), finalBalance);
        }

        emit LiFiTransferCompleted(_lifiData.transactionId, finalAssetId, receiver, finalBalance, block.timestamp);
    }

Recommended Mitigation Steps

Consider adding a reentrancy guard over every function which may send or receive tokens. It may be easiest too add this guard over the fallback() function however that could prevent view functions from being called (since it would perform storage operations).

Ensure the same slot is used to store the reentrancy guard so all required functions are covered by a single guard.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondCutFacet.sol#L14-L22

Vulnerability details

Impact

The owner of the LiFiDiamond contract may set additional facets through the function diamondCut() allowing them to rug pull the protocol by making arbitrary external calls.

Each facet is a smart contract address which may be delegate called (as is the design of the protocol). Since the owner can add new facets and there are no restrictions on what the bytecode is of these new facets. The owner may then later delegate call their new facet. If their new facet is malicious it could transfer all native or other tokens from the contract to the attacker. Furthermore, this attack could be used to spend all allowance of users in the protocol. Since allowances are required to be set by user to use the protocol there is expected to reasonable amount of allowances for the owner to spend.

Proof of Concept

DiamondCutFacet.sol allows adding of new facets through the function LibDiamond.diamondCut() though it may only be called by the owner.

    function diamondCut(
        FacetCut[] calldata _diamondCut,
        address _init,
        bytes calldata _calldata
    ) external override {
        LibDiamond.enforceIsContractOwner();
        LibDiamond.diamondCut(_diamondCut, _init, _calldata);
    }

LiFiDiamond fallback function allows delegatecall to any facet.

    fallback() external payable {
        LibDiamond.DiamondStorage storage ds;
        bytes32 position = LibDiamond.DIAMOND_STORAGE_POSITION;

        // get diamond storage
        // solhint-disable-next-line no-inline-assembly
        assembly {
            ds.slot := position
        }

        // get facet from function selector
        address facet = ds.selectorToFacetAndPosition[msg.sig].facetAddress;
        require(facet != address(0), "Diamond: Function does not exist");

        // Execute external function from facet using delegatecall and return any value.
        // solhint-disable-next-line no-inline-assembly
        assembly {
            // copy function selector and any arguments
            calldatacopy(0, 0, calldatasize())
            // execute function call using the facet
            let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0)
            // get any return value
            returndatacopy(0, 0, returndatasize())
            // return any return value or error back to the caller
            switch result
            case 0 {
                revert(0, returndatasize())
            }
            default {
                return(0, returndatasize())
            }
        }
    }

Recommended Mitigation Steps

Ensure that the owner is a Timelocked DAO rather than an EOA or multisig to prevent the owner from being able to rug pull the protocol.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L150-L171

Vulnerability details

Impact

The function NXTPFacet.swapAndCompleteBridgeTokensViaNXTP() will continue without reverting if the postSwapBalance <= startingBalance.

If the swap results in the balance of the contract decreasing (due to reentrancy bugs or misconfigured _swapData) then transaction will continue without reverting.

This is an issue since there has been a decrease in tokens. This means either the user or the contract is losing tokens.

Proof of Concept

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP()

    function swapAndCompleteBridgeTokensViaNXTP(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        address finalAssetId,
        address receiver
    ) public payable {
        uint256 startingBalance = LibAsset.getOwnBalance(finalAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(finalAssetId);

        uint256 finalBalance;

        if (postSwapBalance > startingBalance) {
            finalBalance = postSwapBalance - startingBalance;
            LibAsset.transferAsset(finalAssetId, payable(receiver), finalBalance);
        }

        emit LiFiTransferCompleted(_lifiData.transactionId, finalAssetId, receiver, finalBalance, block.timestamp);
    }

Recommended Mitigation Steps

This issue may be resolved by ensuring the balance of the contract has increased as seen in the following code. Note there is no benefit to the user if the balance remains the same and so we may use a strict inequality.

    function swapAndCompleteBridgeTokensViaNXTP(
        LiFiData memory _lifiData,
        LibSwap.SwapData[] calldata _swapData,
        address finalAssetId,
        address receiver
    ) public payable {
        uint256 startingBalance = LibAsset.getOwnBalance(finalAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(finalAssetId);

        uint256 finalBalance;

        require(postSwapBalance > startingBalance);
        finalBalance = postSwapBalance - startingBalance;
        LibAsset.transferAsset(finalAssetId, payable(receiver), finalBalance);

        emit LiFiTransferCompleted(_lifiData.transactionId, finalAssetId, receiver, finalBalance, block.timestamp);
    }