Double A-1

In UniswapV2Vault, a liquidity provider can provide capital by calling addLiquidity. In essence, the LP borrows an amount from the asset vault and adds both tokens into Uniswap.

The deposit into Uniswap is done by calling this line:

IUniswapV2Router02(_router).addLiquidity(
    capital, asset, amount, assetToBorrow, 0, 0, address(this), block.timestamp // solhint-disable-line
); 

However, the parameters amountAMin and amountBMin for IUniswapV2Router02(_router).addLiquidity() are hard-coded to 0. This opens any LP deposits to front-running attacks by sandwiching 2 swap transactions before and after. This results in most LP deposits being stolen.

As an illustration, consider a pool with 100 DAI and 100 DBL, and an LP attempting to deposit 100 DAI. The attacker will sandwich the LP’s addLiquidity call by using the following transactions:

1. Front-run a swap transaction of 1000 DAI for 90.90 DBL
2. Back-run a swap transaction of 90.90 DBL back to DAI

The result is the attacker will receive their original deposited amount (1000) and most (~81) of the DAI that the LP deposited.

Remediation To Consider

Remove and parameterize the hard-coded amountAMin and amountBMinvalues in addLiquidity.

When whitelisted, liquidity providers can provide liquidity non-base USD capital tokens. However, when the LP try to remove their liquidity, they will encounter a capital amount mismatch and revert. This is due to conversion to base USD amount relying on the pool ratio, which can change over time.

When an LP adds liquidity, the capitalSupplied amount is stored using ClubManager.addCapital(msg.sender, usd) in the following lines of code:

function addLiquidity(address capital, uint256 amount, address asset) external override nonReentrant {
    ...
    if (_policy.getClubManager() != address(0)) {
      uint256 usd = convertCapitalToUSD(capital, amount);
      IClubManager(_policy.getClubManager()).addCapital(msg.sender, usd);
  }
    ...
}

However, before the capital amount is stored, it is converted to the base USD amount using the Uniswap pool’s ratio using _getQuote:

function _convertCapitalToBaseUSD(address capital, uint256 amount) internal view returns (uint256) {
    ...
    //case#2: there is a trading pair between the capita type and base USD
  address lp = IUniswapV2Factory(_factory).getPair(capital, _policy.getUsdBase());
  if (lp != address(0)) {
            // Gets base USD using the pool ratio
      return _getQuote(amount, capital, _policy.getUsdBase());
  }
    ...
}

When the LP attempts to withdraw, they call removeLiquidity. This time, the amount calculated using the pool ratio is checked against the stored capital amount, which may be greater due to trading activity.

function removeLiquidity(address capital, address asset, uint256 bundleID, uint256 amountAssetIn) external override nonReentrant {
    if (_policy.getClubManager() != address(0)) {
      //convert capitalAmount to whole USD first
      uint256 usd = convertCapitalToUSD(capital, bundle.capitalAmount);
            // removeCapital will check against
      IClubManager(_policy.getClubManager()).removeCapital(msg.sender, usd);
  }
}

A reversion will occur when removeCapital is called:

function removeCapital(address supplier, uint256 amount) external override onlyRole(AMMVAULT_ROLE) {
    // The input amount may be greater than the stored capital suppplied amount from addLiquidity
    require(amount <= capitalSupplied[supplier], "ClubManager: need more capital");
    ...
}

Remediation To Consider

Remove the capital conversion logic and rely on bundle.capitalAmount when removing liquidity.

The ClubManager contract allows Double to control the LP positions users can add and remove by capping them depending on their holdings of DoubleDownClub tokens.

If the ClubManager address is set in the policy contract, every call to addLiquidity() and removeLiquidity() in UniswapV2Vault.sol contract will call ClubManager’s addCapital() and removeCapital() correspondingly. These functions keep track of the capital used (added or removed) inside the capitalSupplied mapping.

function addLiquidity(address capital, uint256 amount, address asset) external override nonReentrant {
        ...
        if (_policy.getClubManager() != address(0)) {
            //convert amount to whole USD first.
            uint256 usd = convertCapitalToUSD(capital, amount);
            **IClubManager(_policy.getClubManager()).addCapital(msg.sender, usd);**
        }
        ...
}

function removeLiquidity(address capital, address asset, uint256 bundleID, uint256 amountAssetIn) external override nonReentrant {
        ...
        if (_policy.getClubManager() != address(0)) {
        //convert capitalAmount to whole USD first
        uint256 usd = convertCapitalToUSD(capital, bundle.capitalAmount);
        **IClubManager(_policy.getClubManager()).removeCapital(msg.sender, usd);**
    }
        ...
}

Otherwise, if the ClubManager address is set to address(0), the logic will continue without making these external calls and without accounting for any capital.

This can cause users who add positions while club manager is disabled not to be able to remove them if the club manager module is enabled afterward.

Remediation To Consider:

Consider flagging liquidity positions that have been added without the club manager module, and bypass the removeCapital() call if so.

The ClubManager keeps track of the capital supplied and prevents a user from supplying more than their capital cap. However, a user can surpass this cap. The reason is due to the use of convertCapitalToUSD to calculate the equivalent base USD amount, and also Solidity’s rounding logic. The calculation depends on the ratio of the Uniswap Pool and the base USD decimal:

function convertCapitalToUSD(address capital, uint256 amount) public view returns (uint256) {
    return (_convertCapitalToBaseUSD(capital, amount) / (10 ** IERC20Metadata(_policy.getUsdBase()).decimals()));
}

The resulting amount of _convertCapitalToBaseUSD is subject to fluctuations due to trading activity. If the asset-to-capital ratio is less than 1, a user can deposit 10 ** IERC20Metadata(_policy.getUsdBase()).decimals() amount of capital to get a rounded down value of 0 and not impact the depositor’s cap.

Remediation To Consider:

Implementing logic to round up to 1 if the calculated amount is less than 1 but greater than 0.

Double protocol does not plan to support fee-on-transfer tokens. However, in UniswapV2LPMigrations.sol, users could mistakenly import existing Uniswap LP positions whose tokens have fee on-transfer. By doing it, every attempt to migrate LP tokens through migrateLPTokens() will revert, since the logic the logic assumes the amounts of tokens returned from Uniswap’s removeLiquidity(), resulting in the LP tokens being stuck.

In UniswapV2Vault.sol, removeLiquidity() function makes multiple external calls to addresses fetched from the AMMVaultPolicy.sol contract:

• _policy.getClubManager()
• _policy.getDoubleDip()
• _policy.getUsdBase()

These three addresses can be configured to new values by administrators. By setting invalid targets in any of these, the removeLiquidity() call would revert, indirectly blocking LP withdrawals.

In UniswapV2Vault.sol, at the end of removeLiquidity, DoubleDip token rewards are calculated using the following logic:

if (_policy.getDoubleDip() != address(0)) {
    uint256 feeInUSD = _convertCapitalToBaseUSD(capital, fee);
    feeInUSD = feeInUSD * (10**36) / (10 ** IERC20Metadata(_policy.getUsdBase()).decimals()); //convert from base USD to 36 decimals
    IDoubleDip(_policy.getDoubleDip()).**earnRewards**(feeInUSD, msg.sender, asset);
} 

Rewards are accumulated using earnRewards(feeInUSD, msg.sender, asset). An admin can intentionally or unintentionally block withdrawals by setting the rewardToFeeRatio to a sufficiently high number (e.g., type(uint256).max). The result is LP providers will be blocked from withdrawing their positions, since the result of earnRewards() will overflow.

Remediation To Consider:

For rewardToFeeRatio, consider capping the rewardToFeeRatio to a maximum equivalent to 100% (DECIMALBASE) or higher if the ratio is intended to overcome this percentage.

In the AMMVaultPolicy.sol contract, the owner has permission to set the addresses of the _clubManager and _doubleDip contracts.

By changing the address of the logic implementation of ClubManager or DoubleDip, the storage values of all variables would be fragmented. In some cases, stored values may be cleared. Not having the stored values in the ClubManager.sol contract can cause:

1. capitalSupplied mapping values: investors will not be able to properly call removeCapital due to their capitalSupply being 0.
2. ddcIdToOwner mapping values: Investors won’t be able to withdraw their DDC tokens if they have used them as an allowance to add liquidity. Otherwise, they will need to withdraw their tokens and send them to the new contract to add liquidity.
3. delegationCount, ddcPowerCount, and ddcOwnedCount: all the delegate counts would be reset.
4. For DoubleDip.sol, the impact is low since the only storage here is rewards statistics, and the contract already has a RewardStorage.sol contract to store unclaimed rewards per user.

Multiple variables are only assigned in the constructor and can be marked as immutable:

In AMMVault.sol:

• _assetVault
• _policy

In ClubManager.sol:

• _ddc

In DoubleDip.sol:

• _rewardStorage
• _assetVault

In RewardStorage.sol:

• _rewardToken

For AssetVault.sol, consider fetching the DoubleDip address from the AMMVaultPolicy contract to keep a unique point of access. This way, by updating the address in a single contract, it will persist on all the other contracts.

For AMMVaultPolicy.sol contract, consider adding events to keep track of important global changes.

In line 37 of UniswapV2Vault.sol, the amount of asset to borrow is checked against the AssetVault reserves. It is, again, checked when calling _assetVault.borrow(asset, assetToBorrow);. Consider removing this duplicate check.