QA Report

[code423n4]

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

[maxklenk]

Thanks for pointing out that there may no token be transferred from the contract to the user, if the swaps are configured like this. We are aware of that and therefore only trigger a transfer if the desired tokens are present in the contract. We still allow this to be able to support swaps which send funds directly to the user.

[gzeoneth]

Downgrading to Low/QA. Treating as warden's QA Report.

[JeeberC4]

Preserving original title: Users May Lose Funds in NXTPFacet.swapAndCompleteBridgeTokensViaNXTP() if the Swap Does Not Increase The Balance