LI.FI contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L29

Vulnerability details

Impact

The from amount in swap must always be transferred from user to the contract. But under certain circumstances contract may end up using its own funds for swapping thereby giving users free funds

Function

swap

Proof of Concept

1. Attacker uses swapTokensGeneric (GenericSwapFacet.sol) to convert non-native asset say DAI to another asset say USDT with _swapData.fromAmount as 100
2. Assume contract already had a balance of amount 100 in DAI
3. The swap ultimately makes calls to swap function in LibSwap.sol#L29
4. Since LibAsset.getOwnBalance(fromAssetId) < fromAmount is false (100<100) so the if condition at L33 does not get executed and no amount is transffered from user
5. Swap is completed and USDT amount gets transffered to user even though user never transferred even a single DAI

Recommended Mitigation Steps

Remove L33-L35 and revise like below:

function swap(bytes32 transactionId, SwapData calldata _swapData) internal {

... // if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {

// }

    if (!LibAsset.isNativeAsset(fromAssetId)) {

LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount); LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount); } .... }

Lines of code

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

Vulnerability details

Impact

The function batchRemoveDex will only remove the first dex index passed in argument and rest in _dexs array will never be removed. This is happening since the function is returning once a matching dex is found instead of breaking from loop

Since function will not revert any error, This can lead to non required dexes still be whitelisted even though owner beleives them to be removed

Function:

batchRemoveDex

Proof of Concept

1. Admin tries to remove dex 1,2 by using batchRemoveDex
2. Loop runs and s.dexWhitelist[1] = false; is set
3. Now Loop is run to find index for s.dexs ==1 and once it is found _removeDex is called which removes 1 from s.dexs
4. But after this return statement executes which completes the function with success even though loop has not yet finished and dex 2 has still not been removed

Recommended Mitigation Steps

Instead of return, kindly use break

 if (s.dexs[j] == _dexs[i]) {
                    _removeDex(j);
                    break;
                }

Lines of code

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

Vulnerability details

Impact

It seems that the approveERC20 function is approving MAX_INT amount instead of the passed amount. This will allow users to spend more than what they are entitled for

Proof of Concept

1. Admin calls approveERC20 to approve amount 5
2. Due to below line an amount of MAX_INT instead get approved for spender

SafeERC20.safeApprove(IERC20(assetId), spender, MAX_INT);

Recommended Mitigation Steps

Change the function line 68 to below:

SafeERC20.safeApprove(IERC20(assetId), spender, amount);

Native funds can get lost

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

1. It was observed that on calling swap function at LibSwap.sol#L29, below calculation is made for previous balance:

uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);

2. This will become wrong if user has set _swapData.receivingAssetId as Native Asset as LibAsset.getOwnBalance will include (original balance + msg.value)

Recommendation:

Change this to:

uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId) - msg.value;

1. It was observed that multiple loop conditions were using postfix increment instead of prefix increment. Kindly consider changing i++ to ++i in order to save gas

2. It was observed that while swapping operations in multiple facet it was not checked whether amount==0. If yes function should immediately return to save gas