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.
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
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.
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
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.7QA - 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
startBridgeTokensViaAnyswaphas addressrouter&recipientthat is missing zero address checkshttps://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L42-L48 Function
initCbridgehas 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
startBridgeTokensViaCBridgehas address inputreceiver, which is lacking zero address checks.https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L17-L26 Function
addDexhas 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
batchAddDexhas 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
diamondCuthas 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
startBridgeTokensViaHophas address inputrecipient, which is lacking zero address checks.https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L124-L140 Function
completeBridgeTokensViaNXTPhas address inputassetId, andreceiverwhich is lacking zero address checks.https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L150-L171 Function
swapAndCompleteBridgeTokensViaNXTPhas address input_cBridgeandreceiverwhich is lacking zero address checks.https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11 Function
transferOwnershiphas 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
_lifiDataare 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
_lifiDatahttps://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
_dexsSuggested 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
initNXTPat https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L33-L37 is an admin called function. HoweverStorage 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 beforeLibDiamond.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 ofStorage 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
_amountSuggested Fix:
Add a require statement to check if
_amountgreater than 0