LI.FI contest - hyh's results

Lines of code

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

Vulnerability details

Impact

All ERC20 tokens can be stolen from the Diamond contract balance via AnyswapFacet, CBridgeFacet, HopFacet, NXTPFacet and GenericSwapFacet (all Facets that inherit from Swapper and call _executeSwaps) as LibSwap's swap utilize the available contract balance instead of user's funds.

This way a malicious user can construct a call so extract any non-native token balance from the Diamond contract. The only requirement is to pass along the crafted _swapData.fromAmount, which is then passed on to LibSwap's swap (after approveTo and callTo verification). I.e. an attacker can set the amount to the observed contract balance of a token and obtain full swap proceeds (setting approveTo and callTo to a valid DEX).

For example, if Bob the attacker observes the Diamond contract to have 3000 DAI on the balance, and let's say ETH is currently costs 3000 DAI. He can send a call with 3000 DAI to ETH swap in _swapData. As the contract balance covers the swap requirement Bob will not be requested for any additional funds, 3000 DAI from the balance be used, and he will obtain 1 ETH minus cumulative transaction cost for free, effectively emptying the DAI balance of the contract.

If the balance usage mechanics is meant to be for mistakenly sent funds utilization, such a design will lead to deterministic loss of those funds, as said Bob will just setup a bot that claims all balance funds as soon as it is economically viable (balance - expected cost > required profit threshold), while user who made a mistake will always be slower to react than said bot.

If 'first send tokens then call' approach is assumed here it has the similar issue as any front running bot will be free to steal the funds sent by invoking the swap before initial user.

Placing the severity to be high as those are direct fund loss scenarios solely enabled by the described issue.

Proof of Concept

LibSwap's swap function use the contract balance instead of user's funds if it is possible:

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

As LibSwap is a library and the code executes in the contract context, the Diamond contract balance will be used and it can be emptied this way.

The attack can be done via several entry points that invoke _executeSwaps and then return back or bridge the swap results to a user:

AnyswapFacet.swapAndStartBridgeTokensViaAnyswap:

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

CBridgeFacet.swapAndStartBridgeTokensViaCBridge:

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

HopFacet.swapAndStartBridgeTokensViaHop:

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

NXTPFacet.swapAndStartBridgeTokensViaNXTP:

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

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP:

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

GenericSwapFacet.swapTokensGeneric:

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

_executeSwaps run all the swaps from _swapData in the cycle via LibSwap.swap(_lifiData.transactionId, _swapData[i]):

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

This way the task for an attacker is to craft _swapData[i] to consist of a sole swap using exactly what the contract holds, obtaining any desired asset to be delivered to attacker's account on another chain.

Recommended Mitigation Steps

As for mistakenly sent funds there is a withdrawal mechanics with the help of WithdrawFacet, consider removing direct usage of own balance for user swap operations.

In the call sequence for the swap-and-bridge case described above the only place where user funds are requested is LibSwap.swap, so there is no workflow that brings in current user's funds to contract balance before that, so any usage of the balance is a transfer from one user to another. As malicious users will use such mechanics in the first place, it is recommended to be disabled.

Now:

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

To be:

if (!LibAsset.isNativeAsset(fromAssetId)) {
    LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
    LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
}

Lines of code

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

Vulnerability details

Impact

All native tokens can be stolen from the Diamond contract balance for AnyswapFacet, CBridgeFacet, HopFacet, NXTPFacet and GenericSwapFacet (all Facets that inherit from Swapper and call _executeSwaps). A malicious user can use any of the swap-invoking functions from these contracts and send a _swapData consisting of an arbitrary large number of the native token swaps, with each using the same msg.value as the cycle invoked by the Swapper parent contract in _executeSwaps call use LibSwap library, and thus have msg.sender, msg.value persisting. First _swapData entry will use the native value sent over by the attacker, while all the subsequent will utilise msg.value from the contract balance as it is an external _swapData.callTo.call from the contract.

For example, if Alice the attacker observes the Diamond contract to have 10 ETH on the balance, and let's say ETH is currently costs 3k DAI. She can send a call with 11 ETH-DAI swaps in _swapData along with msg.value of 1 ETH. She will obtain 11 ETH worth of DAI, i.e. 33k DAI minus swaps cumulative transaction cost, effectively emptying the ETH balance of the contract.

Even if the Diamond contract aren't supposed to have any balance in the normal course of operations, besides mistakenly sent funds case, the user funds that are aimed to be bridged can end up on the contract balance as a consequence of other issues, for example CBridgeFacet's transfer one (it's described separately).

This way there is a possibility that the balance will be substantial and this attack surface opens the way for an attacker to fully retrieve it. As it is a direct fund loss case setting the severity to be high.

Notice, that even if the contract doesn’t hold any excess native token value the reuse of msg.value will lead to a failure for all multi-step operations where the first one involved native tokens swap as there will not be enough funds to execute the second msg.value requiring call.

Proof of Concept

The attack can be done via several entry points that invoke _executeSwaps and then return back or bridge the swap results to a user:

AnyswapFacet.swapAndStartBridgeTokensViaAnyswap:

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

CBridgeFacet.swapAndStartBridgeTokensViaCBridge:

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

HopFacet.swapAndStartBridgeTokensViaHop:

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

NXTPFacet.swapAndStartBridgeTokensViaNXTP:

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

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP:

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

GenericSwapFacet.swapTokensGeneric:

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

_executeSwaps run all the swaps from _swapData in the cycle via LibSwap.swap(_lifiData.transactionId, _swapData[i]):

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

LibSwap is a library, so the execution is done in the same contract context.

LibSwap.swap is sending msg.value from the contract with _swapData.callTo.call:

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

This is done each time on the contract behalf, so these calls will be successfully carried out as long as the contract has enough native balance.

The beneficiary here is the attacker, as the swap resulting funds will be either returned or bridged to her account.

Recommended Mitigation Steps

Consider adding control to _executeSwaps for msg.value to be used strictly once.

If the swaps involve intermediary native tokens then amount will differ from initial msg.value anyway, so the only valid use case for it is the first swap using native amount the user initially sent.

As an example approach, _executeSwaps can pass along a flag to swap indicating that this is the first swap in the sequence and msg.value should be included to the outbound _swapData.callTo.call. If this flag isn't set, the swap is not first and no native tokens should be added to the call.

Lines of code

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

Vulnerability details

Impact

When CBridgeFacet is used with native tokens its core bridging _startBridge function doesn't add the native tokens amount to the CBridge call.

This way either native tokens bridging be unavailable or any native funds sent to the contract be frozen with it and a user lose the control of them until manual recovery, if it be achievable.

Placing the severity to be high as in combination with other issues there is a possibility for user funds to be frozen for an extended period of time (if WithdrawFacet's issue plays out) or even lost (if LibSwap's swap native tokens one also be triggered). In other words the vulnerability is a wider attack surface enabler as it can bring in the user funds to the contract balance.

Proof of Concept

_startBridge does not send native token along with the ICBridge(bridge).sendNative call:

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

As it is an outbound call, the native value to be transferred should be added to it.

Transferring native tokens is a part of functionality:

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

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

Notice that CBridge may or may not revert the call, depending on the implementation.

V2 does check the amounts mismatch and reverts:

https://github.com/celer-network/sgn-v2-contracts/blob/main/contracts/liquidity-bridge/Bridge.sol#L89

But there are no guarantees that such behaviour persists throughout versions, which aren't fixed in LI-FI.

Recommended Mitigation Steps

The corresponding native amount needs to be added to the bridging call, for example:

Now:

ICBridge(bridge).sendNative(...)

To be:

ICBridge(bridge).sendNative{ value: _cBridgeData.amount }(...)

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L70-L75

Vulnerability details

Impact

Only the first DEX is removed in the batch requested as return statement ends the function once the first active DEX from the batch is removed.

Proof of Concept

The return is placed instead of break in batchRemoveDex:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L70-L75

Recommended Mitigation Steps

Consider putting break instead of return.

Also, this code (L70 of DexManagerFacet.sol) can go to a separate private function, say _findAndRemoveDEX:

for (uint256 j; j < s.dexs.length; j++) {
    if (s.dexs[j] == _dexs[i]) {
        _removeDex(j);
        return;
    }
}

Lines of code

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

Vulnerability details

Impact

Any native funds mistakenly sent along with plain ERC20 bridging calls will be lost. AnyswapFacet, CBridgeFacet, HopFacet and NXTPFacet have this issue.

For instance, swapping function might use native tokens, but the functions whose purpose is bridging solely have no use of native funds, so any mistakenly sent native funds to be frozen on the contract balance.

Placing the severity to be medium as in combination with other issues there is a possibility for user funds to be frozen for an extended period of time (if WithdrawFacet's issue plays out) or even lost (if LibSwap's swap native tokens one also be triggered).

In other words the vulnerability is also a wider attack surface enabler as it can bring in the user funds to the contract balance.

Medium despite the fund loss possibility as the native funds in question here are mistakenly sent only, so the probability is lower compared to direct leakage issues.

Proof of Concept

startBridgeTokensViaAnyswap doesn't check that msg.value is zero:

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

startBridgeTokensViaCBridge also have no such check:

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

startBridgeTokensViaHop the same:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L66-L71

In NXTPFacet completion function does the check, but startBridgeTokensViaNXTP doesn't:

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

Recommended Mitigation Steps

Consider reverting when bridging functions with non-native target are called with non-zero native amount added.

Lines of code

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

Vulnerability details

Impact

Any native funds mistakenly sent along with startBridgeTokensViaCBridge bridging call in excess of _cBridgeData.amount will not be used, will remain frozen on the contract balance and can be lost.

Similarly to the zero check issue placing the severity to be medium as in combination with other issues there is a possibility for user funds to be frozen for an extended period of time (if WithdrawFacet's issue plays out) or even lost (if LibSwap's swap native tokens one also be triggered).

Proof of Concept

startBridgeTokensViaCBridge allows msg.value to be greater than _cBridgeData.amount:

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

The excess isn't needed and only _cBridgeData.amount is subsequently used:

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

Recommended Mitigation Steps

Consider reverting when startBridgeTokensViaCBridge is called with any excess native amount.

Now:

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

To be:

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

Lines of code

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

Vulnerability details

Impact

When withdraw function is used with native token it is being handled with a payable.transfer() call.

This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract. This way any programmatical usage of WithdrawFacet is at risk. Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

WithdrawFacet is a core helper contracts that provides basic withdraw functionality to the system, and this way the impact includes principal funds freeze scenario if the described aspect be violated in the DiamondStorage.contractOwner code.

Marking the issue as a medium severity as this is a fund freeze case, but limited to the incorrect contractOwner implementation.

Proof of Concept

When WithdrawFacet's withdraw is called with _assetAddress being equal to NATIVE_ASSET, the native transfer is handled with payable.transfer() mechanics:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L28-L31

WithdrawFacet is a part of EIP-2535 setup:

https://github.com/code-423n4/2022-03-lifinance#diamond-helper-contracts

References

The issues with transfer() are outlined here:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

As withdraw is runnable by the DiamondStorage.contractOwner only the reentrancy isn't an issue and transfer() can be just replaced.

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin's Address.sendValue is advised:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

1. HopFacet's initHop doesn't check the lengths of the argument arrays

Impact

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. System then will fail with low level array access message.

Proof of Concept

HopFacet's initHop:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L40-L50

Recommended Mitigation Steps

Require that _tokens and _bridgeConfigs arrays' lengths match

2. Floating pragma is used in most contracts

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced

Proof of Concept

pragma solidity ^0.8.7 is used in most contracts, for example:

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

Recommended Mitigation Steps

Consider fixing the version to 0.8.x across all the codebase, for example set x to 10

3. Assert is used instead of require in WithdrawFacet

Impact

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message

Proof of Concept

There are two uses of assert:

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

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

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases

4. User facing Facets miss emergency lever

Impact

If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.

Proof of Concept

All core user-facing contracts do not have pausing functionality for new operation initiation.

For example, both AnyswapFacet's user facing functions, bridge and swap-bridge, cannot be temporary paused:

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#L74

Recommended Mitigation Steps

Consider making all end user facing Facets pausable.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

5. No zero-address checks for new owner address

Impact

If Diamond owner be set to zero by mistake the range of system managing functions become unreachable. Particularly, mistakenly sent funds retrieval and DEX white list management become unavailable.

Depending on the stage when it happens it might be needed to redeploy the system.

Proof of Concept

OwnershipFacet.transferOwnership doesn't check the _newOwner:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11

LibDiamond.setContractOwner doesn't check the _newOwner:

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

Recommended Mitigation Steps

Consider requiring _newOwner to be non-zero at one of the levels.

If ownership renounce is in scope it's recommended to add separate methods for it as it's crucial operation for the system that should be more clearly tracked. Also, separate function reduces the probability of setting owner to zero by mistake.

6. Diamond ownership is transferred in one go

Impact

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

OwnershipFacet set a new owner immediately:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11

Recommended Mitigation Steps

Consider utilizing two-step ownership transferring process (proposition and acceptance in separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system

7. DiamondCutFacet can immediately replace any functionality of the system

Impact

Any malfunction can be introduced by the changes either by mistake or with a malicious intent. Say an update can introduce a way to rug the funds.

As this possibility is visible from the contract code it posses the reputational risk even being a part of base Diamond design.

The rationale here is that the project could have, but didn't introduced the measures to mitigate it at least partially.

For example, any integrations and programmatic usage by other projects will be questioned as the very same call can produce quite different results and it's not trivial to setup the checks to control for its validity.

Proof of Concept

DiamondCutFacet's diamondCut let owner to add/remove/replace any functions:

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

Which is done via LibDiamond.diamondCut:

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

Now these calls aren't anyhow constrained, so any change be effective immediately.

Recommended Mitigation Steps

Consider adding transparent constraints on the usage of system upgrade functionality, for example an introduction of two step process with a delay.

If the upgradability is meant to be for alpha-beta stages only consider the introduction of such stages to the code, with disabling the upgrades when the stage progresses past beta, for example.

Further system evolution can be achieved with deployment of additional Diamonds.