Dopex - 0xCiphky's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199

Vulnerability details

Impact

The subtractLoss function within the PerpetualAtlanticVaultLP contract uses collateral.balanceOf(address(this)) under the assumption that it's synchronized with _totalCollateral. However, direct transfers to PerpetualAtlanticVaultLP aren't reflected in _totalCollateral, creating a discrepancy between these two balances.

The settle function in the PerpetualAtlanticVault contract depends on the above function to verify the accurate amount transferred. A mismatch in these balances renders the settle function inoperative.

The subtractLoss function is presented below:

function subtractLoss(uint256 loss) public onlyPerpVault {
        require(collateral.balanceOf(address(this)) == _totalCollateral - loss, "Not enough collateral was sent out");
        _totalCollateral -= loss;
    }

The settle function is as follows:

    function settle(uint256[] memory optionIds)
        external
        nonReentrant
        onlyRole(RDPXV2CORE_ROLE)
        returns (uint256 ethAmount, uint256 rdpxAmount)
    {
	// rest of the code

        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(ethAmount);
        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).unlockLiquidity(ethAmount);
        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(rdpxAmount);

        emit Settle(ethAmount, rdpxAmount, optionIds);
    }

A user can send collateral tokens directly to the PerpetualAtlanticVaultLP contract. When the settle function calls subtractLoss, the require statement will fail because of the imbalance between the active collateral and the actual token balance.

Consequences:

• The mismatch between _totalCollateral and collateral.balanceOf(address(this)) will consistently cause the subtractLoss function to revert, rendering the settle function unusable.

Proof of Concept

Foundry test case

function testBreakSettle() public {
        // initial set up taken from testSettle
        rdpxV2Core.bond(5 * 1e18, 0, address(this));
        rdpxV2Core.bond(1 * 1e18, 0, address(this));

        vault.addToContractWhitelist(address(rdpxV2Core));
        uint256[] memory _ids = new uint256[](1);

        // User mints and transfers 1 WETH to vaultlp contract
        address user1 = vm.addr(1);
        weth.mint(user1, 1 * 1e18);
        weth.transfer(address(vaultLp), 1 * 1e18);

        // test ITM options
        // This will now revert as _totalCollateral is not synced to collateral.balanceOf(address(this)) anymore
        _ids[0] = 0;
        rdpxPriceOracle.updateRdpxPrice(1e7);
        vm.expectRevert(bytes("Not enough collateral was sent out"));
        rdpxV2Core.settle(_ids);
    }

Tools Used

• Manual analysis
• Foundry

Recommended Mitigation Steps

Remove the require validation from the subtractLoss function. Given that the collateral is WETH and the usage of SafeERC20, any transfer failures will inherently cause the settle function to revert. Thus, an additional validation to ensure the successful transfer of funds is redundant.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L324

Vulnerability details

Impact

The recoverERC721 function in the UniV3LiquidityAMO contract is designed to recover ERC721 tokens (NFTs) and transfer them to the RdpxV2Core contract. The transfer is conducted using the safeTransferFrom method, which inherently checks if the receiving contract implements the onERC721Received function. This function acts as a safety mechanism to ensure that the recipient contract can handle ERC721 tokens.

function recoverERC721(address tokenAddress, uint256 token_id) external onlyRole(DEFAULT_ADMIN_ROLE) {
        // Only the owner address can ever receive the recovery withdrawal
        // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported
        INonfungiblePositionManager(tokenAddress).safeTransferFrom(address(this), rdpxV2Core, token_id);
        emit RecoveredERC721(tokenAddress, token_id);
    }

In the scenario presented, the RdpxV2Core contract indeed implements the onERC721Received function. However, the contract lacks the other essential functionalities of the regular ERC721 standard. This deficiency implies that while the RdpxV2Core contract can receive NFTs, it cannot subsequently interact with or transfer them. As a result, any NFTs sent to this contract will become irretrievable.

Tools Used

Manual analysis

Recommended Mitigation Steps

Modify the RdpxV2Core contract to fully implement the ERC721 standard. Ensure that it can not only receive but also interact with and transfer ERC721 tokens.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145

Vulnerability details

Impact

The PerpetualAtlanticVaultLP contract is vulnerable to flash loan attacks. A malicious actor can exploit the distribution mechanism by performing a rapid deposit-withdrawal sequence, allowing them to benefit disproportionately from accumulated collateral.

The root of the problem lies in the different timings of the updateFunding() function call between deposit and withdrawal functions. During a deposit, the shares are calculated before the updateFunding() call. In contrast, during withdrawal, assets are calculated after updateFunding() is invoked. This inconsistency, combined with the potential for accumulated tokens if updateFunding() hasn't been called for a significant period, creates an exploitable scenario.

Scenario

1. The attacker takes a large flash loan.
2. They deposit it into the contract. The shares they receive are based on the contract's current accounting without the latest funding.
3. Immediately after, the attacker triggers a withdrawal. The withdrawal first calls updateFunding(), adding funding.
4. The attacker then redeems their shares, which are now worth more due to the recent update from updateFunding().

Proof of Concept

1. Initial state:

• totalSupply of shares: 100
• _totalCollateral: 1,000
• _rdpxCollateral and its conversion rate: 10 (with a conversion rate of 10)

2. A user takes a flash loan of 500

3. Deposit 500:

When depositing, the formula used is:

• totalVaultCollateral = totalCollateral() + ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
• shares = assets.mulDivDown(supply, totalVaultCollateral)
• totalVaultCollateral = 1000 + (10 * 10)
• shares = 500 × 100 / 1100 ≈ 45 shares

State changes:

• totalSupply increases to 145
• _totalCollateral increases to 1,500
• _rdpxCollateral and its conversion rate: 10 (with a conversion rate of 10)

4. Immediate Redemption:

At the beginning of the redemption process, within the withdrawal function, the updateFunding() function is invoked. If the updateFunding() function hasn't been called for a while and tokens have accumulated, there can be a significant increase in the Collateral. In our scenario, this results in an addition of 200 collateral tokens, taking the total to 1700.

• assets_returned = shares.mulDivDown(totalCollateral(), supply);
• rdpx_returned = shares.mulDivDown(_rdpxCollateral, supply);
• assets_returned = 45 x 1700 / 145 = 527
• rdpx_returned = 45 x 10 / 145 ≈ 3
• _rdpxCollateral * rdpxPriceInAlphaToken = 3 x 10 = 30
• net_gain = 527 + 30 = 557 - 500 = 57

The user was able to drain significant tokens without real participation.

Tools Used

Manual analysis

Recommended Mitigation Steps

Rearrange the operations within the deposit function to update funding prior to calculating user shares:

function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
	perpetualAtlanticVault.updateFunding();
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
	
        // rest of the code
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118

Vulnerability details

Impact

The deposit function in the PerpetualAtlanticVaultLP contract allows users to add collateral to the liquidity pool in exchange for shares. A flaw is apparent where shares for a user are calculated prior to invoking the updateFunding call in PerpetualAtlanticVault. This order of operations results in users receiving more shares than they should, as the calculation is based on a lesser collateral amount present before the updateFunding call.

Proof of Concept

Analyzing the existing deposit function reveals the sequence flaw:

function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

        perpetualAtlanticVault.updateFunding();

        // rest of the code
    }

Within the updateFunding function, there's an evident increase in the contract's collateral:

function updateFunding() public {
    // ... [rest of the code]
    collateralToken.safeTransfer(addresses.perpetualAtlanticVaultLP, (currentFundingRate * (block.timestamp - startTime)) / 1e18);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds((currentFundingRate * (block.timestamp - startTime)) / 1e18);
    // ... [rest of the code]
    }

The shares' computation, being based on the pre-update balances, is effectively outdated at the moment of assignment.

Lets look at the two scenarios and compare the results

Scenario 1: Without updating funding

Starting with the same initial values:

• totalSupply = 1000
• totalCollateral() = 2000
• rdpxPriceInAlphaToken = 2
• _rdpxCollateral = 100
• assets = 100

shares = 100 * 1000 / 2200 = 45 shares

Scenario 2: After updating funding (add 200 to collateral)

• totalSupply = 1000
• totalCollateral() = 2200
• rdpxPriceInAlphaToken = 2
• _rdpxCollateral = 100
• assets = 100

shares = 100 * 1000 / 2400 = 41 shares

Tools Used

Manual analysis

Recommended Mitigation Steps

To rectify this, the order of operations within the deposit function should be modified to ensure the funding gets updated before the user's shares are calculated:

function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
	perpetualAtlanticVault.updateFunding();
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
	// rest of the code
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L975 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L995

Vulnerability details

Impact

The protocol allows users to delegate WETH, which can subsequently be used by others through the bondWithDelegate functionality. Users can manage their delegation using the addToDelegate and withdraw functions as long as the funds are not active collateral.

An oversight in the withdraw function results in the totalWethDelegated not being updated when users withdraw their WETH from delegation.

Below is the withdraw function from the smart contract:

function withdraw(uint256 delegateId) external returns (uint256 amountWithdrawn) {
        _whenNotPaused();
        _validate(delegateId < delegates.length, 14);
        Delegate storage delegate = delegates[delegateId];
        _validate(delegate.owner == msg.sender, 9);

        amountWithdrawn = delegate.amount - delegate.activeCollateral;
        _validate(amountWithdrawn > 0, 15);
        delegate.amount = delegate.activeCollateral;

        IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

        emit LogDelegateWithdraw(delegateId, amountWithdrawn);
    }

Potential attack scenario:

1. Assume the protocol currently has:
   • reserveAsset[reservesIndex["WETH"]].tokenBalance = 1000 WETH
   • totalWethDelegated: 0 WETH
2. A user takes a flash loan for 1000 WETH and invokes the addToDelegate function.
3. The user then calls the withdraw function for 1000 WETH. (The totalWethDelegated should be decremented here but isn't.)
4. Lastly, the user calls the sync function, which updates the WETH reserve balance by the equation:
   • balance = balance - totalWethDelegated;

This results in:

• reserveAsset[reservesIndex["WETH"]].tokenBalance = 0 WETH
• totalWethDelegated: 1000 WETH

NOTE: This issue will happen regardless as users delegate and withdraw their funds but we show an extreme scenario above to simulate the worst case.

Consequences:

• The protocol's reserve accounting for WETH can be wiped due to the missing update on totalWethDelegated.
• While the funds remain within the contract, the accounting mismatch can disrupt most of the protocol's functionality and other contracts that rely on it for funding.
• The protocol would need to be paused, and an emergency withdrawal action would have to be taken to resolve the issue.

Proof of Concept

Foundry test case

function testBreakAccounting() public {
        // lets start with 1000 WETH in reserves and 0 WETH delegated
        weth.mint(address(rdpxV2Core), 1000 * 1e18);
        // call sync function to update contract balances
        rdpxV2Core.sync();
        // lets look at the protocols accounting
	// WETH reserve = 1000 WETH
        (, uint256 wethBalance1,) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertEq(wethBalance1, 1000 * 1e18);

	// User 1 takes flash loan of 1000 WETH
        address user1 = vm.addr(1);
        weth.mint(user1, 1000 * 1e18);
        assertEq(weth.balanceOf(user1), 1000 * 1e18);

        // User adds to delegate with 1000 WETH and X% fee (lets say 99% so other users don't use our collateral)
        vm.startPrank(user1);
        weth.approve(address(rdpxV2Core), 1000 * 1e18);
        uint256 delegateId = rdpxV2Core.addToDelegate(1000 * 1e18, 99 * 1e8);
        vm.stopPrank();

        // lets look at the protocols and users accounting after adding to delegate
        // user balance = 0 WETH
        assertEq(weth.balanceOf(user1), 0);
        // UserDelegated = 1000 WETH
        (, uint256 amount,,) = rdpxV2Core.getDelegatePosition(delegateId);
        assertEq(amount, 1000 * 1e18);
        // totalWethDelegated = 1000 WETH
        assertEq(rdpxV2Core.totalWethDelegated(), 1000 * 1e18);
        // Reserve WETH = 1000 WETH
        (, uint256 wethBalance2,) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertEq(wethBalance2, 1000 * 1e18);

        // User undelegetes 1000 WETH and sync balances
        vm.startPrank(user1);
        rdpxV2Core.withdraw(delegateId);
        rdpxV2Core.sync();
        vm.stopPrank();

        // lets look at the protocols accounting now
        // totalWethDelegated = 1000 WETH
        assertEq(rdpxV2Core.totalWethDelegated(), 1000 * 1e18);
        // Reserve WETH = 0 WETh
        (, uint256 wethBalance3,) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertEq(wethBalance3, 0 * 1e18);
        // User 1 balance = 1000 WETH
        assertEq(weth.balanceOf(user1), 1000 * 1e18);
        // pay back flash loan
    }

Tools Used

• Manual analysis
• Foundry

Recommended Mitigation Steps

Integrate the following line within the withdraw function:

totalWethDelegated -= amountWithdrawn;

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L313 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L372

Vulnerability details

Impact

The RdpxV2Core contract interacts with several other contracts such as UniV2LiquidityAMO, UniV3LiquidityAMO, PerpetualAtlanticVault, and ReLPContract. While funds are transferred between these contracts and RdpxV2Core, most functions update the accounting of the RdpxV2Core contract after this transfer by calling the sync function but not all. This can lead to discrepancies in calculations that rely on these values, such as the bond discount. As a result, users might be offered rates that do not reflect the most recent balances.

Proof of Concept

Certain functions transfer assets to the RdpxV2Core contract but don’t invoke the sync function afterward, which is used to update the internal state:

The following functions transfer funds to the RdpxV2Core contract but don’t call sync after.

1. _sendTokensToRdpxV2Core() in the UniV2LiquidityAMO contract.
2. recoverERC20 in the UniV3LiquidityAMO contract.
3. settle and payFunding in the PerpetualAtlanticVault contract.

This omission can cause inaccurate calculations when determining rates. Given that multiple functions omit the necessary syncing step, it's likely that at some point the reserves used in calculations will be out of date, causing discrepancies.

Tools Used

Manual analysis

Recommended Mitigation Steps

Integrate the sync function into the specified functions or prior to any computations involving reserve assets, guaranteeing that users receive rates based on the latest balances.

Assessed type

Token-Transfer