QuickSwap and StellaSwap contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/2ead456d3603d8a4d839cf88f1e41c102b5d040f/src/core/contracts/AlgebraPool.sol#L608

Vulnerability details

Impact

When a user transfers too much tokens within the _swapCallback, the additional tokens are not reimbursed. It is only required that he transferred at least the amount0 / amount1, but if he transferred more, this will also be accepted (which is generally fine). However, in such a situation, the additional tokens should be reimbursed to the user to avoid overpaying for the swap.

Proof of Concept

• A user performs a USDC/ETH swap and would need to transfer 1000 USDC.
• Instead, he transfers 1050 USDC.
• The swap goes through, but the user does not get back the 50 USDC that were not required for the swap to suceed.

Recommended Mitigation Steps

Reimburse the additional tokens after the swap.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/2ead456d3603d8a4d839cf88f1e41c102b5d040f/src/core/contracts/libraries/DataStorage.sol#L53

Vulnerability details

Impact

The system calculates the volatility over a time period like this:

$\delta(t)=\frac{1}{T} \sum_{\tau \in[t-T, t]}(P(\tau)-\bar{P}(\tau))^{2}$

However, while this estimator is consistent (it converges in probability as the number of samples goes to infinity), it is biased and the produced estimates for finite sample sizes will be generally too low. This will result in fees that are lower than they should be (because the volatility is underestimated) and therefore hurt users.

Recommended Mitigation Steps

Apply Bessel's correction to get an unbiased estimate, i.e.:

$\delta(t)=\frac{1}{T - 1} \sum_{\tau \in[t-T, t]}(P(\tau)-\bar{P}(\tau))^{2}$

• In AlgebraFactory.setOwner, consider implementing a two-step ownership change process (with a confirmation from the new owner). Otherwise, you risk that the owner is set to a wrong address, which will be non-recoverable.
• In AlgebraFactory.createPool, it is checked that token0 (the one with the lower address) is not zero. It would be better to require that token1 (the one with the higher address) is not zero, as this would ensure that both are not zero. Currently, token1 can be zero, as long as token0 is not.
• The system uses uint32 for timestamps. This will only work until February 2106 and the contracts will be unusable afterwards.