The goal of this issue is to show any findings/doubts that I had while reviewing Swapr's zap contracts. The points will be expressed in a list below without any particular ordering. I'll make sure to tick the checkboxes as soon as I think the related issue/doubt has been fixed/resolved.

• Before going the development route, why didn't we have a deep look at Zapper's Uniswap v2 zap contracts for example? Source code for most features is available here and I'm pretty sure they've been audited (at the bare minimum we can't say they haven't been battle tested).

• I'd personally use importing the specific contracts in a module compared to the whole module. Whole-module imports are fine generally but have their downsides (such as not having control over what actually gets imported, which also leads to issues if 2 modules define same-name entities, even if that entity is not directly used in the importer). I find it also results in a cleaner syntax anyway and makes it clear exactly what needs to be imported at all times.

• Instead of using requires with text error messages, use Solidity custom error to reduce gas consumption considerably (parameterless custom errors only take up 4 bytes).

• On line 102, I'm not sure about the logic here, if it is to account for tokens locked in the contract as protocol fee, I'd heavily suggest working with an internal mapping to keep track of the balance instead of doing an extra external call for 2 reasons:

  • It surely is more cheap (the external call cost heavily depends on the ERC20 token implementation, but there's a slight gas overhead to having the external call compared to a SLOAD which is 2.1k gas)
  • This call can be manipulated by a malicious ERC20 token to return any kind of value. This might potentially lead to vulnerabilities and loss of funds.

• On line 108, I'd evaluate adding some unchecked expressions to save a bit on gas. Particularly on the 10000 division (which cannot over/underflow as far as I'm aware). It can also be applied to line 110 since amountFeeTowill always be less or at the very least equal toamountReceived`.

• Code at line 93 could probably be extracted to its own function since it's also used on line 138. I think this also applies to fee transfers (the only difference being that we need to account for the native currency being sent). Maybe we can send the wrapped variant instead of the native currency, having a unifiable ERC20-based logic for the fee transfers.

• On line 247, I'd opt for an infinite approval to the router. If multiple operations are performed by the user with the same token, it might result in a pretty good net saving of gas.

• Operation on line 249 can be wrapped in unchecked.

• On line 316, is there a double approval there when called by _zapInFromToken? I'd remove the approval on one of the 2 places.

• Got to say I'm a bit confused by what's happening at line 326. In case the path is 1 element only, shouldn't we just revert?

• I personally always prefer to define functions as internal when they don't need to be public/external. Gas-wise, it should be the same, but it makes it easier to extend functionality when and if needed.

• I'm not particularly a fan of all the approvals going on here by using the router. I'd consider using a direct, low level approach and interact directly with the pairs in order to avoid approving this much (gas costs should be reduced quite a bit). This can still be simplified by using Uniswap v2's library.

• So, this I'm a bit unsure of in the sense that in the zap contracts I've checked I haven't seen any dedicated logic and we should be protected by this if setting a "good enough" minimum amount out as in not to incur in exaggerated slippage, but there's an occasion in which a user can lose quite a bit of money if they add liquidity after having performed a swap. You see, if the user performs a swap that moves the price (and token ratio consequently) a lot and then provides liquidity, he might get arbed out of a decent chunk of money. Maybe we should take into account the reserves ratio after the swap to zap in, because right now we do input token / 2 and we swap in order to get both tokens in the pair, but we need to be careful because the ratio of the 2 tokens in the target pair changes (even considerably in certain occasions, particularly if the user is careless with slippage settings).

• Linked to the point above, we should be careful with slippage because the user might be sandwiched on one or more of the performed swaps done along the way to get to the final 2 tokens.

On commit 821e7ea

1. Receive native currency only from native currency wrappers: The contract is payable, but it will only receive the native network currency from native currency wrapper contracts, maybe this can be a requirement in the contract.
2. line 280 tokens[i] -> nativeCurrencyAddress.
3. Short unnecessary names in line 294 tokenBal and line 551, there is no problem with using long variable names.
4. Remove the function that removes a supported DEX, we have a setSupportedDex function, and the set function is commonly used to add/remove.
5. _getFeeAndTransferTokens is only used once, remove the function and place the code of it where it is used.
6. Comment is wrong in line 453, the fee is removed if the protocol fee is higher than zero and the sender address is not whitelisted.
7. Proposed refactor to affiliate fees, with the fee being applied over the total amount, like the protocol fee. Also, each affiliate can have its specific fee (it cant be higher than the protocol fee), a deadline for the fee too. So the affiliate status can be removed, the affiliate fee can be applied independently from the protocol fee and it has a deadline. The idea here would be to sell special fee services to other dexes. Dexes can pay X amount of money to have a special fee enabled till a certain date.
8. The transferResidual can be based on an amount, like transferResidualTokenA and transferResidualTokenB, and if the token residual is higher than the specified amount send the residual to the swap executor. This is better than using a boolean and having to estimate the transfer residual that will be left, with this you can specify and only do it when you know you won't spend more gas on the residual transfer than needed. Also, the transferResidual is not tested.
9. Tests against other dexes: Flattened code from other dexes factories and router can be added and generic tests can be run over them.
10. Tests with USDT and KNC tokens. There is a comment on the contact where USDT and KNC tokens are mentioned in line 637, write tests that prove the need for that double approve.
11. No complete tests coverage, more tets should be added to have ~100% tests coverage.