Swapr Zap contracts review - First round

[luzzif]

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.

• [x] 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.

• [x] 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.

• [x] 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 sinceamountFeeTowill 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.

• [x] Operation on line 249 can be wrapped in unchecked.

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

• [x] 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?

• [x] 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.

[luzzif]

Just re-reviewed the contracts after the latest changes. These are some new points:

• [ ] When adding liquiditry it's important to also handle any leftover, and give it back to the user initiating the zap request. As an example let's take this:
  An user wants to zap into USDC/ETH with USDC. The current reserves are 100 USDC and 0.1 ETH, making the current ETH price to be 100 / 0.1 = 1000 USDC. Let's say the user wants to zap in with 20 USDC. Of these 20 USDC, following the contract's logic, 10 are swapped to ETH (swapping happens at lines 251 and 252 of the zap contract). Following the CPMM math, this 10 USDC swap gets the user roughly 0.009 ETH, and so we're ready to add 0.009 ETH and 10 USDC to the pair that has now changed reserves to 100 + 10 = 110 USDC and 0.1 - 0.009 = 0.091 ETH. After our swap the price of ETH in USDC term (which is a representation of the ratio of assets in the pool) is now 110 / 0.091 = 1208.79120879 compared to previously, when it was 1000. The ratio of USDC to ETH that we instead got from our swap to zap in the pool is 10 / 0.009 = 1111.11111111. We can immediately see there's a discrepancy.
  If we check the addLiquidity function of the router (called by the zap contract), we can see that the input values are checked against the current reserves of the pair we want to provide liquidity to (to protect the user from providing liquidity at an unfavourable rate, that would result in immediate arbitrage depending on the specific amount) and either amount A or amount B are adjusted accordingly. Let's run a simulation of the check performed by the router plugging in our numbers, and let's see what happens:
  
• At line 45 of the router contract we can see the reserves of the target pair are fetched.
• At line 49, the router calculates the optimal amount of token B to provide given the amount A desired and the current reserves. The optimal amount is calculated as desiredAmountA * (reserveB / reserveA), which is pretty logical (it calculates the ratio of token B to token A at that point and multiplies it by the desired token A to determine the correct token B amount). If we plug in our numbers, we see that the optimal amount B to provide to the pair given the amount A of 10 USDC is calculated to be 10 * (0.091 / 110) = 0.00827272727273 ETH, slightly less than what we've swapped to.
• At line 73 and 74 we can see that EXACTLY the amounts of tokens determined in the previous step are taken in from the caller (that is, 10 USDC and 0.00827272727273 ETH). This leaves us with 0.009 - 0.00827272727273 = 0.00072727272727 ETH locked in the contract forever, as there's no logic to handle this, and the amount0 and amount1 return values from the router's addLiquidity function are ignored at line 365 of the swap contract.
  While the example is decently extreme, this scenario can happen quite a lot in cases where pairs with new tokens are added to Swapr, liquidity is decently low, but people want to zap in anyway (potentially with decently high slippage tolerance).
  The suggestion in this case would be to either take into account the change in reserves resulting from the swap done to zap into the pool (as previously suggested in the previous comment above, point 13), or at the very least reimbourse the user of any leftovers that can result from adding liquidity at line 365 of the zap contract.
• [ ] Use specific import on both ReentrancyGuard and Ownable in the zap contract.
• [ ] At line 245, I'd probably opt to have only one variable of value amountFrom / 2 to replace amountFromToken0 and amountFromToken1 (you get gas savings in return and at max you can lose 1 wei of value on odd numbers due to integer arithmetic).
• [ ] Not a fan of having to call the pair's token0 and token1 functions at line 385/386. We could probably save on gas by getting the pair's token0 and token1 as inputs to the function and calculating the pair's address dynamically.
• [ ] In the remove liquidity function, there are 2 things I'd add:
  
• At line 390, while calling the router's removeLiquidity, add a minimum amount out check, as anything can happen between the user submitting the zap out transaction and it actually going through, and he could find himself with a different propoertion of tokens compared to what he was expecting.
• At lines 393/394 it's unclear to me why we would use that logic. The removeLiquidity function on the router returns the values that we got by burning the liquidity tokens, why can't we just return those?
• [ ] A few comments about the _getFactoryPair function:
  
• It can probably be rewritten to avoid the factory's static call. We can just calculate the deterministic pair's address and see if there's code in it. It should result in a bit less gas used (EXTCODESIZE is a maximum of 2600 gas while the static call, in this specific context, has a base cost of 2600 + the SLOAD cost in the factory contract, which is 2100).
• Unsure why we use it in the zap in contexts. The function implements a "pair is existing" check while in theory you could very well zap in to a new pair that you are creating in that moment (as the addLiquidity function in the router supports creating the pair dynamically). If the intention is to avoid accidental pair creation, having a separate zapInNewPair function could help.
• [ ] After all, setting the router contract as immutable might have been a mistake, since Uniswap v2 routers are made to be upgradeable (not in the usual meaning of the word, but new ones can be deployed and used).
• [ ] In the zapInFromNativeCurrency, avoiding putting msg.value in memory might result in slight gas savings. (moving in memory results in 1 MSTORE + 1...n MLOADs vs just using the dirt cheap CALLVALUE opcode).