LI.FI contest - SolidityScan's results

Summary:

We found multiple functions which were missing zero address checks. It is advised to add a zero address check at all possible functions setting an address. In solidity, any error caused to set the value to default, and for an address variable, the default value is zero address. Hence any fund or privilege gets pointed to zero address, and it will be unrecoverable. The contract was found to be using floating pragma, which allows the contract to be compiled to multiple versions and hence can cause inconsistencies or errors on different versions. The contract was found to be using a calldata for analytics purposes and did not validate the data. A malicious user could abuse this to pollute the analytics logs with irrelevant data. Another issue was related to an admin function that does not check the array length. In a smart contract, it is always advised to have an array length check when performing a loop operation. We also found some non-critical findings like lack of input validation in amount during the withdraw function. It is recommended to allow withdrawal of greater than 0 value.

Low Severity findings:

QA - 1

Title:

Use of Floating Pragma Version

Description:

The contract was found to be using a floating pragma which is not considered safe as it can be compiled with all the versions described.

PoC:

Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondCutFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondLoupeFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/LiFiDiamond.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibStorage.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L2) Pragma version^0.8.7 (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibUtil.sol#L2)

Suggested Fix:

Use strict pragma version like Pragma version 0.8.7

QA - 2

Title:

Multiple functions Lacking Zero address checks

Description:

Address type parameters should include a zero-address check; otherwise, contract functionality may become inaccessible, or tokens burned forever.

Impact

Tokens may become inaccessible or burnt forever without a zero-address check.

PoC:

Below is the list of functions lacking zero address checks

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35-L66 Function startBridgeTokensViaAnyswap has address router & recipient that is missing zero address checks

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L42-L48 Function initCbridge has address input _cBridge, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L57-L84 Function startBridgeTokensViaCBridge has address input receiver, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L17-L26 Function addDex has address input _dex, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L30-L40 Function batchAddDex has address input _dexs, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondCutFacet.sol#L14-L21 Function diamondCut has address input _init, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L61-L87 Function startBridgeTokensViaHop has address input recipient, which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L124-L140 Function completeBridgeTokensViaNXTP has address input assetId, and receiver which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L150-L171 Function swapAndCompleteBridgeTokensViaNXTP has address input _cBridge and receiver which is lacking zero address checks.

• https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11 Function transferOwnership has address input _newOwner, which is lacking zero address checks.

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

Suggested Fix:

Address zero address check to all the missing places.

QA - 3

Title:

Event pollution

Description:

The values inside _lifiData are not being validated in any of the functions. This data is being used for tracking, analytics, and monitoring. The functions in which the lifiData are being used are external or public. These functions can be called by external users concurrently, due to which the event logs will be polluted if a malicious actor enters random data inside the _lifiData.

Impact

A malicious actor can pollute event logs by concurrently calling the function and emitting random events.

PoC:

This affects all the contract using the user supplied calldata input _lifiData

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

Suggested Fix:

Validate the calldata value against the value actually processed rather than any user-supplied value.

QA - 4

Title:

No limit on the input array length

Description:

Ethereum is a very resource-constrained environment. Prices per computational step are orders of magnitude higher than with centralized providers. Moreover, Ethereum miners impose a limit on the total number of gas consumed in a block. If array.length is large enough, the function exceeds the block gas limit, and transactions calling it will never be confirmed. This becomes a security issue, if an external actor influences array.length. E.g., if array enumerates all registered addresses, an adversary can register many addresses, causing the problem described above.

PoC:

Go to https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L30-L40 we will notice there is no length validation for array _dexs

Suggested Fix:

Either explicitly or just due to normal operation, the number of iterations in a loop can grow beyond the block gas limit which can cause the complete contract to be stalled at a certain point. Therefore, loops with big or unknown number of steps should always be avoided.

---

Non-critical findings

QA - 5

Title:

LibDiamond.enforceIsContractOwner() should be called before in initNXTP function

Description:

The initNXTP at https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L33-L37 is an admin called function. However Storage storage s = getStorage(); is called before the admin function which would be usless if the caller is not an admin.

PoC:

Notice Storage storage s = getStorage(); being called before LibDiamond.enforceIsContractOwner();

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

Suggested Fix:

LibDiamond.enforceIsContractOwner(); can be called before to avoid unessary execution of Storage storage s = getStorage();

QA - 6

Title:

Missing input validation in amount

Description:

The contract does not check if the value of amount is greater than 0.

PoC:

Go to https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L23 we will notice there is no input validation for _amount

Suggested Fix:

Add a require statement to check if _amount greater than 0

Summary for gas optimization

The contract was found to be using struct variables in random order. Since structs use packing and assign storage slots accordingly, lower to higher-order will save gas as lower variables get packed. We found many functions were declared as public but were never internally used. Such functions can always be marked as external instead of public as external functions consume less gas than public functions. We also noticed the contract uses power operations at a few places instead of using the exponential form directly, which would save gas.

Gas-1

Title:

Structs can be rearranged from lower to higher to save gas.

Description:

If the variable types in the struct are arranged in a lower to a higher order, the gas used is lesser than when they are arranged in any other order. The main goal here is the reduction of gas requirements when deploying these smart contracts, and saving costs in each place will eventually add up. The consequences of the use of the Tight Variable Packing pattern have to be evaluated before implementing it blindly. The big benefit comes from the substantial amount of gas that can potentially be saved over the lifetime of a contract.

PoC:

At line (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L26-L33) We will notice that the structs are ordered as below

address receiver;
address token;
uint256 amount;
uint64 dstChainId;
uint64 nonce;
uint32 maxSlippage;

This will consume 370235 gas

However the struct is arranged in lower to higher order. The lower ones will get packed in a single slot and hence it will save gas.

uint32 maxSlippage;
uint64 dstChainId;
uint64 nonce;
uint256 amount;
address receiver;
address token; 

The above arrangement will consume 370199 gas

Suggested Fix:

Arrange the struct variable from lower to higher order.

Gas-2

Title:

Functions that can be external instead of public

Description:

Public functions that are never called by a contract should be declared external in order to conserve gas.

Impact

Smart Contracts are required to have effective Gas usage as they cost real money, and each function should be monitored for the amount of gas it costs to make it gas efficient.

Public functions cost more Gas than external functions.

PoC:

The following functions can be declared external:

1. Function setRewardForwarding (https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L298-L302)
2. Function swapAndStartBridgeTokensViaAnyswap (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L74-L123)

3. Function startBridgeTokensViaCBridge (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L57-L84)
4. Function swapAndStartBridgeTokensViaCBridge (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L92-L134)
5. Function swapTokensGeneric (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22-L44)
6. Function startBridgeTokensViaHop (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L61-L87)
7. Function swapAndStartBridgeTokensViaHop (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L95-L126)
8. Function startBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46-L76)
9. Function swapAndStartBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85-L115)
10. Function completeBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L124-L140)
11. Function swapAndCompleteBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L150-L171)
12. Function withdraw (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L20-L38)

Suggested Fix:

Use the external state visibility for functions that are never called from the contract.

Gas-3

Title:

Use of power operations instead of their exponential form

Description:

It is unnecessary to perform power operations when a number can be directly represented in an exponential form as it will save gas.

PoC:

1. At https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L15 2e256 can be used instead of 2**256
2. At https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L8 2e256 can be used instead of 2**256

Suggested Fix:

Use exponential form instead of power operations to save gas.