LI.FI contest - ych18's results

Lines of code

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

Vulnerability details

• DexManagerFacet.batchRemoveDex is used to remove many DEXs in the same transaction. However, the return in L73, will push the function to return directly after removing the first DEX in the _dexs list. So the actual implementation will just remove the first element in the list, which could be a serious problem as the contract owner could think that he removes an access to a DEXs while he doesn't. I put this as high because this affects the ACCESS CONTROL.

• Recommendation: remove the return (as simple as that).

• Incorrect revert msg in NXTPFacet.completeBridgeTokensViaNXTP. In fact, the native token could be in different blockchain (e.g Polygone and the native token is MATIC)

• The transfer ownership system ( OwnershipFacet )does not follow for transfer-accept patterns. Which may lead to contract ownership accidentally being mismanaged. Recommendation: Implementing a transfer-accept pattern, or renouncing ownership of the contract may be preferred to countermeasure against mistaken ownership transfers.

• it's better to replace the NATIVE_ASSET to another address rather than the address(0) to avoid any confusion that could happen (e.g WithdrawFacet.withdraw)

• All contracts use an unlocked pragma ^0.8.7.

• Function is declared as public but is only called externally. This occurs in a couple of instances:

• NXTPFacet: startBridgeTokensViaNXTP, swapAndStartBridgeTokensViaNXTP, completeBridgeTokensViaNXTP and swapAndCompleteBridgeTokensViaNXTP
• WithdrawFacet: withdraw
• GenericSwapFacet: swapTokensGeneric
• AnyswapFacet: swapAndStartBridgeTokensViaAnyswap, swapAndStartBridgeTokensViaAnyswap

• The unchecked block can be used for all the for loops. Consider adding an internal function _increment with an unchecked block and use it to increment the counter inside the for loop.

• LibAsset.decreaseERC20Allowance and LibAsset.increaseERC20Allowance are declared but never used