Simplify `_depositAndSwapERC20` to save some gas

[sujithsomraaj]

Context: GenericSwapFacet.sol#L275

Description: The function _depositAndSwapERC20 approves the swapData.approveTo address if there is not enough allowance (or) allowance is zero. However the function can be simplified to save some gas.

-        if (currentAllowance == 0) {
-            // just set allowance
-            ERC20(_swapData.sendingAssetId).safeApprove(
-                _swapData.approveTo,
-                type(uint256).max
-            );
-        } else if (currentAllowance < _swapData.fromAmount) {
+       if (ERC20(_swapData.sendingAssetId).allowance(
+            address(this),
+            _swapData.approveTo
+        ) < _swapData.fromAmount
            // allowance exists but is insufficient
            // reset to 0 first
            ERC20(_swapData.sendingAssetId).safeApprove(
                _swapData.approveTo,
                0
            );
            // then set allowance
            ERC20(_swapData.sendingAssetId).safeApprove(
                _swapData.approveTo,
                type(uint256).max
            );
        }

[PASS] test_CanSwapSingleERC20ToERC20_V2() (gas: 245244) ==> New Logic
[PASS] test_CanSwapSingleERC20ToERC20_V2() (gas: 245280) ==> Current Logic

Recommendation: Consider simplifying the logic to save approximately 36 GAS.

LI.FI:

Researcher:

[0xDEnYO]

I dont fully understand the recommendation. This is changing the logic of the code, not a pure gas optimization. We have three cases:

• no allowance set (case 1)
• insufficient allowance set (case 2)
• sufficient allowance set (case 3)

And we wanted to treat all these three cases separately. So are you suggesting to remove the "currentAllowance" variable to save gas here? Cause your code also removes the branch for case 1 and that would result in a waste of gas since we set allowance to 0 although it is 0 already.

Please comment

[sujithsomraaj]

Ok this is quite tricky. Cuz the above will safe if there is enough approval. But if there is not enough approval, then it'll be costlier. But the non-appoval case is rare, as we set it to max.

[0xDEnYO]

I see your point

How about this one instead?

        if (currentAllowance < fromAmount) {
            // check if is non-zero
            if (currentAllowance != 0) sendingAsset.safeApprove(approveTo, 0);
            sendingAsset.safeApprove(approveTo, type(uint256).max);
        }

[sujithsomraaj]

This should be ideal, I guess. efficient in both cases.

[0xDEnYO]

Awesome....will go forward with this. With closing this last issue we can consider this audit done.

Appreciate your support and professionalism. Thanks, Sujith.