LI.FI contest - BouSalman's results

QA Report for Li.Fi protocol

This report do list low-risk and non-critical findings presented in the Li.Fi protocol codebase, these findings do not effect any assets connected to users or the protocol. However these issues are related to coding and security best practices.

Findings

Unused events

There is many declared events in the protocol codebase but not used to fire any emits, In that case any off-chain monitoring routines can not log changes done to the protocol's state. It's advised to emit these events when needed or remove it from the codebase to have a clear and consistent code.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ILiFi.sol#L40 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ILiFi.sol#L51 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L142 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L144 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L147 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L149 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L152 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L154 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L157 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L166 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Interfaces/ITransactionManager.sol#L177

Numbers presentation

When dealing with big numbers developers can make a use of scientific notation format to increase code readability, However there is occurrences in Li.Fi codebase where double asterisk is used instead of scientific notation.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L8 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L15

Unused imported files

There are some unused imports in the protocol, this would decrease code quality and effect code audit time. it's advised to remove it before deploying the protocol.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L6 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L9

Commented lines of code

There is a commented lines of code that define a struct of Facet, However this struct is already defined and used in another file. it's advised to remove the comment line to have a concise and clear codebase.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondLoupeFacet.sol#L13-L16

Missing zero address checking

There is multiple instances inside the Li.Fi protocol where the logic do not validate for zero address when assigning value, it's best practice to sanitize addresses before saving them in the state since there is a possibly to interrupt protocol business or loss of funds.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/LiFiDiamond.sol#L9 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/LiFiDiamond.sol#L16 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L45 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondCutFacet.sol#L20 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L137 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L167 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/OwnershipFacet.sol#L10 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L49 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L47 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L80

Floating pragma for compiler version

Avoid using floating pragmas for compiler version inside the the source code of Li.Fi protocol, lock the compiler version for the Code files and leave it floated for libraries if needed. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/LiFiDiamond.sol#L2 and *all* files under /src folder

Usage of Deprecated Library Functions

Avoid using Deprecated Library Functions, for example in the Li.Fi codebase there is a use of safeAprove() function which is discouraged. it's advised to use the safeIncreaseAllowance() function or the already defined function in the file increaseERC20Allowance()

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L67-L68

Inconsistent coding style for if statements

it was notice that the developers of the Li.Fi do use a shortcut coding technique when using IF statements inside the functions code which is a good and concise way to coding. But in multiple occasions the shorthanded ? is used and then the oneliner IF statement is used, it's better to stick to one style of shortcut when using simple IF statements to have a clear and consistent style of coding. for example: [the ? case] https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L38 [oneliner IF case] https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L64