Dopex - Tendency's results

Lines of code

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

Vulnerability details

Impact

Whenever proceeds is sent to PerpetualAtlanticVaultLP, the function PerpetualAtlanticVaultLP::addProceeds is called to update the stored _totalCollateral value.

When called, the function ensures that the contract collateral token balance is greater than or equal to the sum of the stored _totalCollateral value plus the proceeds, but on settlement PerpetualAtlanticVault::settle function makes an external call to PerpetualAtlanticVaultLP::subtractLoss function, this function updates the variable _totalCollateral by subtracting the realized loss.

The problem here is, this function requires that the PerpetualAtlanticVaultLP collateral balance matches the stored totalCollateral minus the realized loss. This implementation can easily be abused by an attacker, by sending a small amount of collateral token directly to the PerpetualAtlanticVaultLP contract address. This would allow the addition of proceeds, but it would make settling options impossible because the subtractLoss function would consistently fail at the require check

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

consider making the following change to the subtractLoss function:

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When a writer deposits via PerpetualAtlanticVaultLP::deposit function, the function previewDeposit is called internally, this function further calls convertToShares function, this function returns the amount of shares a user will receive for a given amount of assets. The problem here is, the computation in convertToShares function makes use of an outdated _totalCollateral variable, which results in wrongly calculating user shares.

  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(); // <--- Here

    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    _totalCollateral += assets;

    emit Deposit(msg.sender, receiver, assets, shares);
  }

Notice that perpetualAtlanticVault::updateFunding function is called after conversion from asset to shares. Note that, perpetualAtlanticVault::updateFunding function, transfers funding into the perpetualAtlanticVaultLP in a drip-vested manner. Whenever perpetualAtlanticVault::updateFunding function is called to update and transfer funding, perpetualAtlanticVaultLP::addProceeds function is also called, addProceeds function updates the stored _totalCollateral value based off the amount sent from the vault to the perpetualAtlanticVaultLP, hence, it is important to ensure the right _totalCollateral value is used in computation by first updating funding, since this value is constantly changing.

As confirmed by a sponsor(psytama), funding was meant to be updated before calling the function previewDeposit, as rightly done in perpetualAtlanticVaultLP::redeem function.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When a user wishes to bond via RdpxV2Core::bond or RdpxV2Core::bondWithDelegate, the function calculateBondCost is called, this function calculates and returns how much weth and rDPX token is required to bond.

Assuming the user transaction is executed at a block.timestamp equal to the next funding payment timestamp, timeToExpiry variable in RdpxV2Core::calculateBondCost will be zero

    function calculateBondCost(
        uint256 _amount,
        uint256 _rdpxBondId
    ) public view returns (uint256 rdpxRequired, uint256 wethRequired) {

           #############

        uint256 timeToExpiry = IPerpetualAtlanticVault(
            addresses.perpetualAtlanticVault
        ).nextFundingPaymentTimestamp() - block.timestamp;
        if (putOptionsRequired) {
            wethRequired += IPerpetualAtlanticVault(
                addresses.perpetualAtlanticVault
            ).calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
        }
    }

This variable is used in calculating premium via a call to PerpetualAtlanticVault::calculatePremium. The problem here is, time expiry will be calculated using a not up to date next funding payment timestamp, thereby making the calculated premium, different from the premium calculated in PerpetualAtlanticVault::purchase function, this is possible since before calculating and updating wethRequired in RdpxV2Core::calculateBondCost function, PerpetualAtlanticVault::updateFunding function isn't called, when called this function will update latestFundingPaymentPointer given current timestamp equals or is greater than nextFundingPaymentTimestamp, while In PerpetualAtlanticVault::purchase function, updateFunding is called, which then updates the latest funding payment. Hence the premium calculated here, will differ from the premium user deposited, making the contract pay up user premium under such conditions.

 function purchase(
    uint256 amount,
    address to
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 premium, uint256 tokenId)
  {
    _whenNotPaused();
    _validate(amount > 0, 2);

      //SNIP, Took out outer logic for easy viewing

    updateFunding();


    uint256 timeToExpiry = nextFundingPaymentTimestamp() - block.timestamp;

    // Get total premium for all options being purchased
    premium = calculatePremium(strike, amount, timeToExpiry, 0);

    // Transfer premium from msg.sender to PerpetualAtlantics vault
    collateralToken.safeTransferFrom(msg.sender, address(this), premium);

    perpetualAtlanticVaultLp.lockCollateral(requiredCollateral);
    _updateFundingRate(premium);



    emit Purchase(strike, amount, premium, to, msg.sender);
  }

Assuming funding duration is 7 days, where the genesis timestamp is set to 1694127600, when latestFundingPaymentPointer was previously zero(i.e. First epoch), latestFundingPaymentPointer will be 1 after PerpetualAtlanticVault::updateFunding function is called nextFundingPaymentTimestamp will be = 1694127600 + (1 * 7 days) ==> 1694732400 The difference between current timestamp and nextFundingPaymentTimestamp is then used to calculate the expiry. This creates a disparity between the amount user paid and the amount PerpetualAtlanticVault transfers from RdpxV2Core contract.

PS: This Report is written based off current scope, if IOptionPricing::getOptionPrice, which according to a sponsor isn't yet deployed reverts on 0 time expiry, this will mean a DoS to users bonding at a time when block.timestamp == nextFundingPaymentTimestamp, which will still need to be fixed.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

update funding payment pointer before calculating bond cost

        IPerpetualAtlanticVault(addresses.perpetualAtlanticVault).updateFundingPaymentPointer();

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Currently, the redeem function in RdpxV2Core allows users to withdraw DSC from a matured bond without checking for the contract's paused state. In a case where the admin wants to stop all activity, e.g. In the event of a hack or compromise, they will be unable to stop the transfer of receipt tokens to users via RdpxV2Core::redeem function.

In the rise of such events, being able to halt vital functionalities is important to mitigate further risk/fund loss

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Add the whenNotPaused modifier to RdpxV2Core::redeem function

Assessed type

Context