Security Audit
January 20th, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
be522ba55229c70e6e5085507d5b3cedd4adf4c4
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
contracts/AMMVault.sol |
|
contracts/AMMVaultPolicy.sol |
|
contracts/AssetVault.sol |
|
contracts/ClubManager.sol |
|
contracts/DoubleDip.sol |
|
contracts/DoubleDipJoy.sol |
|
contracts/DoubleDownClub.sol |
|
contracts/DoubleDownClubDescriptor.sol |
|
contracts/LpBundles.sol |
|
contracts/RewardStorage.sol |
|
contracts/UniswapV2LPMigration.sol |
|
contracts/UniswapV2Utils.sol |
|
contracts/UniswapV2Vault.sol |
|
contracts/interfaces/IAMMVaultPolicy.sol |
|
contracts/interfaces/IAssetVault.sol |
|
contracts/interfaces/IClubManager.sol |
|
contracts/interfaces/IDoubleDip.sol |
|
contracts/interfaces/IDoubleDipJoy.sol |
|
contracts/interfaces/IDoubleDownClub.sol |
|
contracts/interfaces/IDoubleDownClubDescriptor.sol |
|
contracts/interfaces/IRewardStorage.sol |
|
contracts/interfaces/IUniswapV2LPMigration.sol |
|
contracts/interfaces/IUniswapV2Vault.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
We quantify issues in three parts:
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. |
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:
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 amountBMin
values in addLiquidity
.
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.
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:
capitalSupplied
mapping values: investors will not be able to properly call removeCapital
due to their capitalSupply
being 0
. 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. delegationCount
, ddcPowerCount
, and ddcOwnedCount
: all the delegate counts would be reset.Double does not intend to migrate ClubManager after launch.
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.
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.