QA Report

[code423n4]

Title: Assert instead require to validate user inputs Severity: Low Risk

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.

    WithdrawFacet.sol : reachable assert in line 33
    WithdrawFacet.sol : reachable assert in line 29

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

    OwnershipFacet.sol.transferOwnership _newOwner

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    LibAsset.sol: function transferNativeAsset parameter recipient isn't used. (transferNativeAsset is internal)
    LibAsset.sol: function transferNativeAsset parameter amount isn't used. (transferNativeAsset is internal)

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base

Deprecated safeApprove in LibAsset.sol line 66: if (allowance > 0) SafeERC20.safeApprove(IERC20(assetId), spender, 0);

Deprecated safeApprove in LibAsset.sol line 67: SafeERC20.safeApprove(IERC20(assetId), spender, MAX_INT);

Deprecated safeApprove in LibSwap.sol line 37: LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);

Deprecated safeApprove in Swapper.sol line 15: ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    IERC173.sol
    OwnershipFacet.sol

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    LibAsset.sol.getOwnBalance assetId
    LibAsset.sol.approveERC20 spender
    LibAsset.sol.transferFromERC20 assetId
    LibAsset.sol.increaseERC20Allowance spender
    LibAsset.sol.transferERC20 assetId
    DexManagerFacet.sol.removeDex _dex

[H3xept]

Re: assert -> require

Fixed in previous commit.

[H3xept]

Re Onwership:

Fixed in lifinance/lifi-contracts@221fe883f705635feebb2af64a028f30d05afbf8

[H3xept]

Re zero owner

Duplicate of #192

[H3xept]

Re Assert is used instead of require

Duplicate of #165