comfyswap-solo.md

Audit Report - ComfySwap

Audit Date	08/16/2023
Auditor	HollaDieWaldfee (@HollaWaldfee100)
Version 1	08/16/2023 Initial Report
Version 2	08/16/2023 Mitigation Review

Contents

• Audit Report - ComfySwap
• Contents
• Disclaimer
• About HollaDieWaldfee
• Scope
• Centralization
• Severity Classification
• Summary
• Findings
  • High Risk Findings (1)
    • 1. ComfySwapV2Pair: unsafe cast to uint128 can lead to loss of LP tokens
  • Improvement Findings (2)
    • 2. ComfyLpLocker: improve getLps and getPairLocks view functions
    • 3. ComfyLpLocker: refactor lockLiquidity function

Disclaimer

The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.

Furthermore, this report is not an endorsement or certification of the smart contract, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.

About HollaDieWaldfee

HollaDieWaldfee is a top ranked Smart Contract Auditor doing audits on code4rena (www.code4rena.com) and Sherlock (www.sherlock.xyz), having ranked 1st in multiple contests.
On Sherlock he uses the handle "roguereddwarf" to compete in contests.
He can also be booked for conducting Private Audits.

Contact:

Twitter: @HollaWaldfee100

Scope

The audit has been conducted in the sponsor's private repository (https://github.com/ComfySwap/comfyswap-contracts) at commit f23a3fc70431e8110827bd6093b81c014440ec5e.

The project is a fork of UniswapV2.

The main in-scope contract is ComfyLpLocker which is not forked from UniswapV2 and implements a mechanism for timelocking LP tokens.

In order to integrate ComfyLpLocker with UniswapV2, changes have been made to UniswapV2Pair (ComfySwapV2Pair) and UniswapV2Factory (ComfySwapV2Factory) which have also been reviewed.

Centralization

The centralization concern from the original report has been addressed in commit 6547650ac0bd5b67491fb3d004eb456ad88132d9.
The lpLocker address can no longer be updated.

What now remains is the ability for the feeToSetter address to enable 0.05% of the swap fees to go to the protocol instead of the liquidity providers. While this can lower the reward for liquidity providers (especially since there is the timelocking mechanism) there is no risk for loss of funds as such. Therefore we can say the centralization risk has been largely mitigated.

Severity Classification

Severity	Impact: High	Impact: Medium	Impact: Low
Likelihood: High
Likelihood: Medium
Likelihood: Low

Impact - the technical, economic and reputation damage of a successful attack

Likelihood - the chance that a particular vulnerability is discovered and exploited

: Findings in this category are recommended changes that are not related to security but can improve structure, usability and overall effectiveness of the protocol.

Summary

Severity	Total	Fixed	Acknowledged	Disputed	Reported
	1	1	0	0	0
	0	0	0	0	0
	0	0	0	0	0
	2	1	1	0	0

#	Title	Severity	Status
1	ComfySwapV2Pair: unsafe cast to uint128 can lead to loss of LP tokens
2	ComfyLpLocker: improve getLps and getPairLocks view functions
3	ComfyLpLocker: refactor lockLiquidity function

Findings

High Risk Findings (1)

1. ComfySwapV2Pair: unsafe cast to uint128 can lead to loss of LP tokens

Description:
The ComfySwapV2Pair contract casts the amount of LP tokens that are minted to the ComfyLpLocker contract to uint128 (Link).

The ComfySwapV2Pair contract does not provide any assurance that liquidity <= type(uint128).max.
Also the Solidity version 0.5.16 does not provide any overflow checks by default.

This means that there can be an overflow and the amount of LP tokens in the lock is lower than the actual amount of LP tokens minted (with the difference being type(uint128).max).

This leads to a loss of LP tokens for the liquidity provider.

Impact:
As agreed upon with the sponsor, I will not provide a quantitative assessment of this issue.
The fix is straightforward and my reasoning for why the fix is safe follows in the next section.

A basic scenario is when LP token to reserves has an exchange rate of 1:1.

Let's look at the following code from ComfySwapV2Pair:

if (_totalSupply == 0) {
    liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
   _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
} else {
    liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);
}

If token0 and token1 of the pair have very low valuation, 2^128 = ~10^38 might not be much denominated in USD (say $1k) and so it's perfectly possible for someone to deposit on the order of 10^38 of token0 and token1.

The liquidity to mint would be 10^38 which is in the order of magnitude that triggers the overflow.
Now griefing attacks can occur as well with people front-running each other.

So there is a clear path for a loss of funds.

Recommendation:
The cast to uint128 can simply be removed.

diff --git a/contracts/ComfySwapV2Pair.sol b/contracts/ComfySwapV2Pair.sol
index d878449..b3fc936 100644
--- a/contracts/ComfySwapV2Pair.sol
+++ b/contracts/ComfySwapV2Pair.sol
@@ -127,7 +127,7 @@ contract ComfySwapV2Pair is IUniswapV2Pair, ComfySwapV2ERC20 {
         }
         require(liquidity > 0, 'UniswapV2: INSUFFICIENT_LIQUIDITY_MINTED');
         _mint(address(lpLocker), liquidity);
-        lpLocker.lockLiquidity(to, uint128(liquidity));
+        lpLocker.lockLiquidity(to, liquidity);
         _update(balance0, balance1, _reserve0, _reserve1);
         if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
         emit Mint(msg.sender, amount0, amount1);

diff --git a/contracts/ComfyLpLocker.sol b/contracts/ComfyLpLocker.sol
index 82a4762..cf9b668 100644
--- a/contracts/ComfyLpLocker.sol
+++ b/contracts/ComfyLpLocker.sol
@@ -42,7 +42,7 @@ contract ComfyLpLocker is IComfyLpLocker {
     factory = _factory;
   }
 
-  function lockLiquidity(address _owner, uint128 _amount) external {
+  function lockLiquidity(address _owner, uint256 _amount) external {
     require(_isPair(msg.sender), 'ComfyLpLocker: INVALID_PAIR');
     uint256 expireTime = locks[msg.sender][_owner].expireTime;
     if ( expireTime > 0) {

Why is there no risk of overflow then in the ComfyLpLocker contract?
We can observe the following: The liqudity variable is also used in the call to _mint. _mint employs overflow checks as it makes use of the SafeMath library when updating its balances.

The sum of all LP token balances cannot exceed type(uint256).max.

The amounts of LP token that are tracked in the ComfyLpLocker contract are obviously capped at the total supply of LP tokens.

As all LP token amounts in ComfyLpLocker are stored in uint256 variables there can be no overflow.

Fix:
The issue has been fixed in commit 6547650ac0bd5b67491fb3d004eb456ad88132d9 by implementing the recommendation.

Improvement Findings (2)

2. ComfyLpLocker: improve getLps and getPairLocks view functions

Description:
There are three changes that I propose for both functions:

• change access modifiers to external
• optimize gas-usage in for-loops
• cap _limit at the length of the set (ensures that there's no revert when calling with a _limit that is too big)

Recommendation:

diff --git a/contracts/ComfyLpLocker.sol b/contracts/ComfyLpLocker.sol
index 82a4762..bfbed5d 100644
--- a/contracts/ComfyLpLocker.sol
+++ b/contracts/ComfyLpLocker.sol
@@ -85,17 +85,29 @@ contract ComfyLpLocker is IComfyLpLocker {
     return allLps[_pair].length();
   }
 
-  function getLps(address _pair, uint256 _start, uint256 _limit) public view returns(address[] memory) {
+  function getLps(address _pair, uint256 _start, uint256 _limit) external view returns(address[] memory) {
+    uint256 lpsLength = allLps[_pair].length();
+    require(_start < lpsLength, 'ComfyLpLocker: INVALID_START_INDEX');
+    if (_start + _limit > lpsLength) {
+      _limit = lpsLength - _start;
+    }
+
     address[] memory lps = new address[](_limit);
-    for (uint256 i = 0; i < _limit; i++) {
+    for (uint256 i; i < _limit; ++i) {
       lps[i] = allLps[_pair].at(_start + i);
     }
     return lps;
   }
 
-  function getPairLocks(address _pair, uint256 _start, uint256 _limit) public view returns(OwnedLock[] memory) {
+  function getPairLocks(address _pair, uint256 _start, uint256 _limit) external view returns(OwnedLock[] memory) {
+    uint256 lpsLength = allLps[_pair].length();
+    require(_start < lpsLength, 'ComfyLpLocker: INVALID_START_INDEX');
+    if (_start + _limit > lpsLength) {
+      _limit = lpsLength - _start;
+    }
+
     OwnedLock[] memory pairLocks = new OwnedLock[](_limit);
-    for (uint256 i = 0; i < _limit; i++) {
+    for (uint256 i; i < _limit; ++i) {
       address lp = allLps[_pair].at(_start + i);
       Lock memory lock = locks[_pair][lp];
       pairLocks[i] = OwnedLock(lp, lock.amount, lock.expireTime);

Fix:
Both functions have been declared external in commit 6547650ac0bd5b67491fb3d004eb456ad88132d9.

The remaining suggestions have not been implemented based on the reasoning that these functions will only be queried from the UI, not from other protocols on-chain which might experience unexpected reverts.

In addition, when not all data can be fetched with one function call, inconsistent data might be fetched due to how the underlying AddressSet is implemented. Namely, it does not provide any ordering guarantees.

As discussed with the sponsor, the UI will soon make use of an event-based monitoring of Liquidity Providers and Locks, moving away from the view functions.

The above considerations make me estimate that it's a fair decision to not make any further changes to both view functions, so we can safely mark this as "acknowledged".

3. ComfyLpLocker: refactor lockLiquidity function

Description:
The lockLiquidity function can be simplified as some of the code is redundant.

Recommendation:

diff --git a/contracts/ComfyLpLocker.sol b/contracts/ComfyLpLocker.sol
index 82a4762..85f9058 100644
--- a/contracts/ComfyLpLocker.sol
+++ b/contracts/ComfyLpLocker.sol
@@ -48,15 +48,14 @@ contract ComfyLpLocker is IComfyLpLocker {
     if ( expireTime > 0) {
       // existing lock, we are just increasing the locked amount
       locks[msg.sender][_owner].amount += _amount;
-      emit LiquidityLocked(msg.sender, _owner, _amount, expireTime);
     } else {
       // new lock
-      uint128 unlockTime = uint128(now + INITIAL_LOCK_TIME);
-      Lock memory newLock = Lock(_amount, unlockTime);
+      expireTime = now + INITIAL_LOCK_TIME;
+      Lock memory newLock = Lock(_amount, expireTime);
       locks[msg.sender][_owner] = newLock;
       allLps[msg.sender].add(_owner);
-      emit LiquidityLocked(msg.sender, _owner, _amount, unlockTime);
     }
+    emit LiquidityLocked(msg.sender, _owner, _amount, expireTime);
   }

Fix:
The recommended change has been implemented in commit 6547650ac0bd5b67491fb3d004eb456ad88132d9.