LI.FI contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

Here: https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L42, it should be fromAmount , otherwise it can be really misleading for users and decrease the flexibility as you can only have 1 swap (the first) involving native asset.

Proof of Concept

Assume Alice wants to swap 1 ETH for 3000 USDC in first swap using Uniswap, and then 1 ETH for 3000 USDC in a second swap using Sushiswap, all the eth would be send to Uniswap !

Another case would be if Alice wants to first swap USDC to DAI, and then ETH to DAI, all ETH would be sent to the first swap !

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

Comments here are misleading:

"Performs a swap and that's it" whereas you can pass an array of swap and perform multiple operations.

"an array of swap related data for performing swaps before bridging" whereas the function is external so could be used without any bridging

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

Code could be reorganized to reuse the same check and be more readable:

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