LI.FI contest - hake's results

Lines of code

LibSwap.swap swapTokensGeneric

Vulnerability details

Impact

Users could input incongruent values for _lifiData and _swapData leading to a swap no being processed correctly and users not getting any of the expected _lifiData.receivingAssetId.

It can also damage reputation because LiFi could use this to scam their users through a front end by changing inputs deliberatly.

Proof of Concept

• User calls swapTokensGeneric but accidentally inputs two different addresses for _lifiData.receivingAssetId(DAI) and _swapData.receivingAssetId(ADA).

function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData)

• LibSwap.swap uses _swapData.receivingAssetId(ADA) to make the swap, however GenericSwapFacet.swapTokensGeneric.postSwapBalance uses _lifiData.receivingAssetId(DAI) to calculate the amount to be transferred based on its balance before the swap.

uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);

vs

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

• The balance of DAI has not changed on LiFi, so the user will not get any, however the swap for ADA was successful and was sent to LiFis contract instead.

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

Tools Used

Recommended Mitigation Steps

Implement a check to make sure _lifiData and _swapData values are the same.

Lines of code

LibSwap.swap GenericSwapFacet.sol

Vulnerability details

Impact

Remaining or unaccounted ERC20 balance could be freely taken through swapTokensGenerics and swap.

Proof of Concept

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

        toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId) - toAmount;

Given:

• There has been a deposit to LiFi of a non-native ERC20 that makes LibAsset.getOwnBalance(fromAssetId) a desirable amount.

• Attacker calls swapTokensGeneric with a _swapData.fromAmount value just below LibAsset.getOwnBalance(fromAssetId).

• First if statement in swap is skipped (no funds are tranferred to LiFis contract).

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

• Swap happens and increases LibAsset.getOwnBalance(_lifiData.receivingAssetId)
• Difference of LiFis balance of the receiving token before and after swap is calculated using postSwapBalance and transfered to attacker.

Tools Used

Recommended Mitigation Steps

Ensure funds are always subtracted from users account in swap, even if LiFi has enough balance to do the swap.

Lines of code

LibSwap.swap: L29-46 LibAsset.approveERC20

Vulnerability details

Impact

If a whitelisted dex gets compromised or becomes ill intentioned it can withdraw all funds in LiFi via approveERC20 and/or a reentrancy attack through the LibSwap.swap function. By the time the dex is blacklisted it might be too late, as there are no inbuilt boundaries or mechanisms to mitigate an attack. This means that LiFi is only as secure as the dexs it allows/approves.

Proof of Concept

LibSwap.swap: L29-46

The call in line 42 (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData); has no reentrancy guard an could be used as a reentrancy vector if any approved callTo turned malicious.

LibAsset.approveERC20

The LibAsset.approveERC20 function allows any approveTo address to withdraw as much as they want from LiFi with no limits or boundaries as we can see in the last three lines of code below.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a reentrancy guard at swapAndStartBridgeTokensViaNXTP and similar functions. I recommend using ReentrancyGuard from OpenZeppelin. Set allowance dynamically equal to the amount to be transfered/swapped instead of an unlimited amount. Implement checks for making sure _swapData and _nxtpData are congruent with each other.

Lines of code

DiamondCutFacet.sol WithdrawFacet.sol DexManagerFacet.sol

Vulnerability details

Impact

contractOwner has complete freedom to change any functionality and withdraw/rug all assets. Even if well intended the project could still be called out resulting in a damaged reputation like in this example

Proof of Concept

https://twitter.com/RugDocIO/status/1411732108029181960

Tools Used

Recommended Mitigation Steps

Recommend implementing extra safeguards such as:

• Limiting the time period where sensitive functions can be used.
• Having a waiting period before pushed update is executed.
• Using a multisig to mitigate single point of failure in case contractOwner private key leaks.

LOW

Low#1: initNXTP can be initialized multiple times.

L33-37

function initNXTP(ITransactionManager _txMgrAddr) external {
        Storage storage s = getStorage();
        LibDiamond.enforceIsContractOwner();
        s.nxtpTxManager = _txMgrAddr;
    }

The function initNXTP has init in its name, suggesting that it should only be called once to intiliaze the nxtpTxManager. However, it can be called multiple times to overwrite the address.

Recommend setting the address in a constructor or reverting if address is already set. Third option would be changing the name from initNXTP to something like setNXTP to better align function names with their functionality.

Note: Same issue is present in initHop and initCbridge.

Low#2: No zero value checks for both _nxtpData.amount and msg.value in startBridgeTokensViaNXTP.

L46-60

function startBridgeTokensViaNXTP(LiFiData memory _lifiData, ITransactionManager.PrepareArgs memory _nxtpData)
        public
        payable
    {
        // Ensure sender has enough to complete the bridge transaction
        address sendingAssetId = _nxtpData.invariantData.sendingAssetId;
        if (sendingAssetId == address(0)) require(msg.value == _nxtpData.amount, "ERR_INVALID_AMOUNT");
        else {
            uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);
            LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _nxtpData.amount);
            require(
                LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance == _nxtpData.amount,
                "ERR_INVALID_AMOUNT"
            );
        }

Lack of zero value check on both _nxtpData.amount and msg.value allow bridge to be wastefully started.

Recommend implementing a require function such as(line 50):

require(msg.value > 0 || _nxtpData.amount > 0, "Amount must be greater than zero!");

Low#3: No events when approving/blocking a DEX contract.

DexManagerFacet.sol

Implementing events here would increase transparency and trust.

Low#4: No zero address check for adding DEX contract in addDex, batchAddDex, removeDex and batchRemoveDex.

addDex

Zero address check would prevent adding harmful DEX contracts by mistake. If zero address DEXs are whitelisted, users could burn their tokens on accident.

Low#5: Unchecked transfer in WithdrawFacet.sol

WithdrawFacet.sol

Boolean return value for transfer is not checked.

assert(_amount <= self.balance);
            payable(sendTo).transfer(_amount);
        } else {

I recommend implementing call instead:

(bool success, ) = sendTo.call.value(_amount)("");
require(success, "Transfer failed.");

Low#6: Function swapTokensGeneric incorrect as to spec.

swapTokensGeneric

* @param _lifiData data used purely for tracking and analytics

The above statement in the code snippet is incorrect because _lifiData is used to calculate the amount to be transfered back to user after swap.

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

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

I recommend either exchanging the use of _lifiData for _swapData or changing the comments.

NON-CRITICAL

Non-crit#1: initNXTP and initHop emit no event.

NXTPFacet.sol: L33-37 HopFacet.sol: L40-52 Emitting an event with the initialization of nxtpTxManager and initHop can increase the protocols transparency and trust. CbridgeFacet.initCbridge emits an event, so keeping it consistent is also good practice.

Non-crit#2: Minor typo in comment in line 31. Conatains - Contains.

L31

Fix typo.

Non-crit#3: Implementation of startBridgeTokensViaCBridge has same functionality but deviates from conventions set in startBridgeTokensViaHop and startBridgeTokensViaNXT

L57-84 L61-72

By comparing startBridgeTokensViaCBridge to startBridgeTokensViaHop and startBridgeTokensViaNXT we can see that the first deviates from the other two, despite containing the same functionality. This hurts readbility. Please implement startBridgeTokensViaCBridge in the same manner the other startBridge functions have been implemented. Differences can be seen below.

startBridgeTokensViaCBridge:

function startBridgeTokensViaCBridge(LiFiData memory _lifiData, CBridgeData calldata _cBridgeData) public payable {
        if (_cBridgeData.token != address(0)) {
            uint256 _fromTokenBalance = LibAsset.getOwnBalance(_cBridgeData.token);

            LibAsset.transferFromERC20(_cBridgeData.token, msg.sender, address(this), _cBridgeData.amount);

            require(
                LibAsset.getOwnBalance(_cBridgeData.token) - _fromTokenBalance == _cBridgeData.amount,
                "ERR_INVALID_AMOUNT"
            );
        } else {
            require(msg.value >= _cBridgeData.amount, "ERR_INVALID_AMOUNT");
        }

startBridgeTokensViaHop:

function startBridgeTokensViaHop(LiFiData memory _lifiData, HopData calldata _hopData) public payable {
        address sendingAssetId = _bridge(_hopData.asset).token;

        if (sendingAssetId == address(0)) require(msg.value == _hopData.amount, "ERR_INVALID_AMOUNT");
        else {
            uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);
            LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _hopData.amount);
            require(
                LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance == _hopData.amount,
                "ERR_INVALID_AMOUNT"
            );
        }

Non-crit#4: Commented code in DiamondLoupeFacet.sol.

DiamondLoupeFacet.sol

Please delete code snippet below as it serves no purpose.

// Diamond Loupe Functions
    ////////////////////////////////////////////////////////////////////
    /// These functions are expected to be called frequently by tools.
    //
    // struct Facet {
    //     address facetAddress;
    //     bytes4[] functionSelectors;
    // }

Non-crit#5: Incosistent return type within DiamondLoupeFacet.sol contract.

supportsInterface

The function supportsInterface uses an unamed return, while the rest of the contract uses named returns. Please keep it consistent within contracts and accross the project if possible to improve readibility.

function supportsInterface(bytes4 _interfaceId) external view override returns (bool) {
        LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
        return ds.supportedInterfaces[_interfaceId];
    }

Gas#1: Unecessary read from storage prior to a possible revert in initNXTP.

L33-37

        Storage storage s = getStorage();
        LibDiamond.enforceIsContractOwner();
        s.nxtpTxManager = _txMgrAddr;
    }

Storage storage s = getStorage(); reads from storage even though the function can revert at LibDiamond.enforceIsContractOwner(); causing unnecessary usage of gas.

Recommend implementing LibDiamond.enforceIsContractOwner(); prior to Storage storage s = getStorage();

Example:

        LibDiamond.enforceIsContractOwner();
        Storage storage s = getStorage();
        s.nxtpTxManager = _txMgrAddr;
    }

Gas#2: Unnecessary else statement in swapAndStartBridgeTokensViaCBridge

L92-119

The use of the else statement below is not necessary and can be avoided by implementing swapAndStartBridgeTokensViaCBridge in the same manner swapAndStartBridgeTokensViaHop or swapAndStartBridgeTokensViaNXTP have been implemented. Reference for swapAndStartBridgeTokensViaHop below.

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

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

Note: Same issue present in AnyswapFacet.swapAndStartBridgeTokensViaAnyswapL74-108