radev_sw-Q.md

[QA-01] Well implementation can be destroyed

Summary

Aquifer is Factory contract for deploying Well contracts. The Aquifer creates a single implementation of Well and then creates a proxy to that implementation every time a new Well contract needs to be deployed, but don't initialize it, which means that anybody can initialize the contract.

The Wll.sol contract is provided on the attack vector surfaces where someone could everytime frontrun it by monitoring the mempool. The init() function can be called by anyone who monitors the mempool. A malicious attacker could monitor the blockchain for bytecode that matches the Well contract or the init() function signature and frontrun the transaction. This act can be repeated in a way similar to a Denial Of Service (DOS) attack, effectively stopping contract deployment. This could lead to the failure of the project plan and result in unrecoverable gas costs.

Current implementation of Aquifer constructor

    constructor() ReentrancyGuard() {}

Recommendation

There are two ways:

• Add init() in Aquifer.sol constructor
• Add init() in Well.sol constructor (and make the initialize() function public instead of external)

[QA-02] Array Length Mismatch in _addLiquidity function

Summary

The _addLiquidity function accepts an array tokenAmountsIn as a parameter, representing amounts of different tokens to be added to the liquidity pool. This array is processed in a loop where it interacts with another array, _tokens, which holds the actual token contracts.

However, the function does not contain a necessary check to ensure that the length of tokenAmountsIn matches the length of _tokens. This can lead to unpredictable and potentially damaging behavior in the following scenarios:

• If tokenAmountsIn is longer than _tokens: The loop will continue to iterate beyond the length of _tokens, leading to failed transactions or potential fallback to the zero address. This could lead to loss of user funds.

• If tokenAmountsIn is shorter than _tokens: Some tokens will not have a corresponding input amount, possibly leading to an imbalance in the reserves. This might be exploited to impact the integrity of the liquidity pool.

Recommendation

Ensure the lengths of tokenAmountsIn and _tokens match. The following check should be added at the beginning of the function:

require(tokenAmountsIn.length == _tokens.length, "Mismatched array lengths");

[QA-03] lpAmountOut can be zero

Summary

The function _addLiquidity calculates the amount of liquidity pool (LP) tokens to be minted (lpAmountOut) based on the reserves and the amounts of tokens being added. The function proceeds to mint and transfer LP tokens even when the lpAmountOut is zero.

While this does not result in unexpected behavior or security vulnerabilities, it does unnecessarily consume gas for minting and transferring zero LP tokens, and it can cause confusion when reading and interpreting contract events.

Recommendation

Add:

require(lpAmountOut > 0, "No liquidity tokens to mint");

[QA-04] Use different approach to check for ERC20 token duplicates

Summary

You can use mapping to check for ERC20 token duplicates in init() function in Well.sol contract

        mapping(uint256 => bool) seenTokens;

        for (uint256 i = 0; i < _tokens.length; i++) {
            if (seenTokens[_tokens[i]]) {
                revert DuplicateTokens(_tokens[i]);
            }
            seenTokens[_tokens[i]] = true;
        }

Using a mapping to check for duplicates in a single pass with a time complexity of O(n) is more efficient than nested loops with a time complexity of O(n^2), particularly when the size of the array is large.

1. Time Complexity:

• The nested loops approach takes O(n^2) time because for each element in the array, it compares it with almost every other element in the array. So, if there are n elements, it will perform roughly n * n = n^2 comparisons.
• The mapping approach takes O(n) time because it iterates through the array once and uses a constant time operation (hash map lookup/insertion) to check for duplicates.

Let's take an example:

• For an array of size 100:

• The nested loops approach will take approximately 100 * 100 = 10,000 operations.

• The mapping approach will take approximately 100 operations.

• For an array of size 1000:

• The nested loops approach will take approximately 1000 * 1000 = 1,000,000 operations.

• The mapping approach will take approximately 1000 operations.

2. Gas Costs:

• In Ethereum, computational operations cost gas, and gas is directly related to real money (Ether). Having fewer operations (as in the mapping approach) will generally cost less gas, especially when the array sizes are large.

3. Performance:

• As the array size grows, the nested loops approach will take exponentially more time to execute than the mapping approach. This can lead to slower transaction times and potentially even transaction failures if the gas limit is exceeded.

In conclusion, the mapping approach is much more efficient in terms of execution time and gas costs, especially when dealing with large datasets. It is generally a good practice to prefer algorithms with lower time complexity when writing smart contracts to optimize for performance and gas costs.

[QA-05] Add maxAmountOut function parameter for additional slippage protection

Summary

In the current version of the swapFrom() function, the output amount (i.e., amountOut) is only checked against a minimum threshold (minAmountOut). This approach could lead to unexpected outcomes if the amount of toToken to be received is higher than anticipated. This issue could occur due to price fluctuations, and it might impact users who have strict balance requirements or those whose token positions could be affected negatively by receiving an unexpectedly large number of tokens.

A maxAmountOut parameter could be added to the swapFrom() function to provide an upper limit for amountOut. This adjustment could offer a second layer of control against slippage and afford users more predictability.

The same additional slippage check can be done for shift(), swapFromFeeOnTransfer() addLiquidity() and removeLiquidity function.