Reach out for an audit or to learn more about Macro
or Message on Telegram

Double A-1

Security Audit

January 20th, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Double's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from December 19, 2022 to December 28, 2022.

The purpose of this audit is to review the source code of certain Double Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 2 - - 2
Medium 1 - - 1
Low 1 - - 1
Code Quality 3 - 3 -
Informational 4 2 - 2
Gas Optimization 1 - 1 -

Double was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Contract SHA256
contracts/AMMVault.sol

c63456d2356e15f385c9d94de025160f8044e2692ed6b271f73f82cc50b17f0c

contracts/AMMVaultPolicy.sol

e6ca5e092f8b453774b8bcb967016e6f7445710b00ad6b8497118857a4bd4563

contracts/AssetVault.sol

0da36903494a6e69ea5754305c037713ec9d90e4a13a20edbd702947b1c23331

contracts/ClubManager.sol

c4c0a1d86777d25fcecd5a04146052ddc3cd913ac457d0223d99737ec99861a0

contracts/DoubleDip.sol

818dd87f5d3a931e41618d73fce7a25ad1ee43ce766a201a68ccde041b3d29d4

contracts/DoubleDipJoy.sol

d97b824cdb54d1923a889351165b1c919ab651e8285b17fe10f835d27d27b533

contracts/DoubleDownClub.sol

c08fe5ebdbd8d920fac2f5912bb9ee2c1fe84f4e18bb9dc76943c72e5d39309e

contracts/DoubleDownClubDescriptor.sol

cddfdba0c4f1fb3966b2f07c2884dcb528293076661cbc8e96dedba30d8f4120

contracts/LpBundles.sol

4354aa4d79b031b9a543dea85e4822822a612bbe9f5f7899350ffeb7fded68fa

contracts/RewardStorage.sol

109e23cc2024e5b00a45846c41d0713c14764fa141e02c77aa7cfe1ce251c26a

contracts/UniswapV2LPMigration.sol

5b8f4d16927282bb19a9931214ef00adfb1b327a2cdf49293a8083f7b74c22a7

contracts/UniswapV2Utils.sol

5bfb824cac4819b0dbb847341b72b3ca9d2d8f0af2374003a8b5ff4e27505908

contracts/UniswapV2Vault.sol

821a81e58ad796c9aeea105623f02f7c0ffcdfa718ff888421d062d5954f3478

contracts/interfaces/IAMMVaultPolicy.sol

0b896feaa7aae67761a68bfb407bf81a10d76546af6b9781f327b35631adabd2

contracts/interfaces/IAssetVault.sol

8d2b2dfe1b4b6bbcd28c81e8b3a6abcfdadc5fdc140c1a807fa4e10198e178a0

contracts/interfaces/IClubManager.sol

e3e4899dcf89897cdf66aecde2ed5104876cfeb694b7347ca2b73e2ce09c385e

contracts/interfaces/IDoubleDip.sol

6466403db3009e38cebc94d5a591d9de9377ce87799d91f77544ba1993d3b975

contracts/interfaces/IDoubleDipJoy.sol

0543979d1d7f8acfe9726210b65bc8ef540bd4fb6c4a594f6448a67e9a66bcac

contracts/interfaces/IDoubleDownClub.sol

d7441fdf191dd23eec191662584d3de6d5c3445cc1d1926a5647560e94eb32d6

contracts/interfaces/IDoubleDownClubDescriptor.sol

38b614f79e3f003423e8045e6348fb3ed9c9cb8e6d34442df7ca2f555ccb03b0

contracts/interfaces/IRewardStorage.sol

ec20addbf1824d8090a04ec165232c6933380fdd3a92e851f2e080899e501d59

contracts/interfaces/IUniswapV2LPMigration.sol

5b2c2b714ee1fed474136246d355cd9df58597535acd2c6ff6f7f6c2a41e3abc

contracts/interfaces/IUniswapV2Vault.sol

4d6e69042da7d7cfaeca81f1872e7bb13a7e3b4d937e20577b3062ae898a9d8e

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Unsafe hard-coding of Uniswap min values causes LP token loss

Topic
Front-runing
Status
Impact
High
Likelihood
Medium

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.

Response by Double

Currently, there is really no solution to eliminate this type of attack for AMM. Uniswap does not solve this. And Double can't solve this either. This is an active research area in AMM and MEV to find the optimum price range check. Users should be comfortable with the risk and proceed with caution to use Double and AMM in general.

Double has optioned to add a price range check, for example 0.5%, can limit the method to pump the token price used by attackers. However, if the liquidity supply is still large enough, the transaction could still be sandwich attacked. In addition, an attacker can mount a DoS attack to pump the token price by 0.5% which will cause the transaction to revert, a bad UX for the product. When the liquidity provider tries to add liquidity again after the revert at a higher price, the attacker can mount a DoS attack again and pump the token price by another 0.5%. The attacker can repeat the DoS attacks a few times and then allow the liquidity provider to add liquidity at a much higher token price. Then the attacker dumps which cause the same type of loss for the liquidity provider.

H-2

LP withdrawals with non-base USD capital can be blocked

Topic
Data Model
Status
Impact
High
Likelihood
Medium

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.

M-1

Differentiated logic for ClubManager can offset capital accounting

Topic
Trust Model
Status
Impact
Medium
Likelihood
Low

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.

L-1

LP can deposit above capital supply cap

Topic
Use Cases
Status
Impact
Spec Breaking
Likelihood
Low

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.

I-1

Fee-on-transfer tokens can get stuck

Topic
Trust Model
Impact
Informational

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.

I-2

Invalid addresses configuration can block LP withdrawals

Topic
Trust Model
Status
Acknowledged
Impact
Informational

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.

I-3

Unbounded fee ratio can block LP withdrawals

Topic
Trust Model
Status
Impact
Informational

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.

I-4

Dangerous logic migration

Topic
Trust Model
Status
Acknowledged
Impact
Informational

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.
Response by Double

Double does not intend to migrate ClubManager after launch.

Q-1

Variables can be marked as immutable

Topic
Code Quality
Status
Wont Do
Quality Impact
Low

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
Q-2

AssetVault.sol: DoubleDip address could be fetched from policy contract

Topic
Code Quality
Status
Wont Do
Quality Impact
Low

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.

Q-3

No events in global config contract

Topic
Events
Status
Wont Do
Quality Impact
Low

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

G-1

Unnecessary check of asset reserve

Topic
Gas Optimization
Status
Wont Do
Gas Savings
Low

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.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Double team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.