-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
46 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
# [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 initialize() 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 initialize() 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 | ||
```solidity | ||
constructor() ReentrancyGuard() {} | ||
``` | ||
|
||
### Recommendation | ||
There are two ways: | ||
- Add initialize() in Aquifer.sol constructor | ||
- Add initialize() 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: | ||
```solidity | ||
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: | ||
```solidity | ||
require(lpAmountOut > 0, "No liquidity tokens to mint"); | ||
``` |