OasisSwap security record

The `OasisSwapFactory` and `OasisSwapRouter` contracts both contain the `setRebateEstimator` function, which allows the setting of the rebate estimator address. However, an update in one does not necessitate an update in the other. There is a possible scenario that both `OasisSwapFactory` and `OasisSwapRouter` contracts will refer to different rebate estimator contracts upon reconfiguration. Having ability to configure the rebate estimator address independently at both contract leads to potential desync issues when calling `OasisSwapFactory.getRebate()` versus `OasisSwapRouter.getRebate()`.

The [Checks-Effects-Interactions](https://solidity.readthedocs.io/en/latest/security-considerations.html#use-the-checks-effects-interactions-pattern) coding pattern is meant to mitigate any chance of other contracts manipulating the state of the blockchain in unexpected and possibly malicious ways before control is returned to the original contract. As the name implied, only after checking whether appropriate conditions are met and acting internally on those conditions should any external calls to, or interactions with, other contracts be done. The following instance that violates this pattern has been identified in `OasisSwapPair.withdrawFee()`: ```=304 if (_send0) { _safeTransfer(token0, _to, _toSend0); <-- external call _toSend0 = 0; } if (_send1) { _safeTransfer(token1, _to, _toSend1); <-- external call _toSend1 = 0; } feeCache0 = _toSend0; <-- state change happens after external call feeCache1 = _toSend1; <-- state change happens after external call ``` The severity of this issue has been set to informational, because the re-entrancy guard has been properly implemented throughout the contract, mitigating the issue. However, it's best to adhere to this pattern throughout the code life cycle.

Division before multiplication may result in a loss of precision when the operations are carried over integer numbers in Solidity. Specifically the following calculations within the internal `_swap` function of the `OasisSwapPair` contract can result in a loss of precision: ``` oasisFee0 = feeTaken0.div(FEE_DIVISOR).mul(_oasisFeeProportion).div(FEE_DIVISOR); oasisFee1 = feeTaken1.div(FEE_DIVISOR).mul(_oasisFeeProportion).div(FEE_DIVISOR); ```

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.8.*`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked".

In a comment on top of the `_mintFee` function of the `OasisSwapPair` contract, it is stated that the function should "mint liquidity equivalent to 1/6th of the growth in sqrt(k)". The code to calculate the amount to mint is different from the specification. There is an example of the formula from the actual code implementation: <img src="https://github.com/pavel-quantstamp/test/blob/main/Screenshot%202023-06-09%20at%2008.35.21.png?raw=true" alt="actual-code-implementation" width="200"/> When there is an example of the formula from technical specification: <img src="https://github.com/pavel-quantstamp/test/blob/main/Screenshot%202023-06-09%20at%2008.31.41.png?raw=true" alt="technical-specification" width="200"/> Therefore, the actual fees in this system are lower than 1/6th of the growth in sqrt(k) because the denominator in the implemented formula is larger than 6 * sqrt(kLast) given sqrt(kLast) < sqrt(k). The code comments stating the function mints liquidity equivalent to "1/6th of the growth in sqrt(k)" are therefore misleading. So, it seems that either the code comments or the code implementation are incorrect, as the two are not consistent. As such, it's important to reconcile the two for accurate understanding and implementation of the desired functionality.

In the current system, functions such as `OasisSwapRouter.getAmountsOut()` and `OasisSwapRouter.getAmountsIn()` have become user-specific. This is because these functions now calculate the fee based on the msg.sender address, which means the results vary depending on the user making the call.