Dopex - juancito's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L201 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

Vulnerability details

Summary

The PerpetualAtlanticVaultLP::subtractLoss() function has a strict balance check with a require.

By providing any amount of collateral to it, like 1 wei, the function will always revert. This function is used by the PerpetualAtlanticVault::settle() function, which is called by the RdpxV2Core::settle() function.

This will make the settle function always revert.

Impact

With a DOS of the RdpxV2Core::settle() function, no option can be settled. This can be undone as the strict balance check on the substractLoss() function will always make the function revert.

The attack is extremely cheap, and easy to exploit, just requiring the attacker to send 1 wei of collateral to the PerpetualAtlanticVaultLP contract at any time.

Proof of Concept

The root of the finding is this strict equality check.

Once an attacker sends any amount of collateral tokens to the contract, the require statement will not pass, making the function always revert.

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

PerpetualAtlanticVaultLP.sol#L201

This function is called by the PerpetualAtlanticVault on its settle() function:

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

PerpetualAtlanticVault.sol#L359-L361

And originally, PerpetualAtlanticVault::settle(), is called within the Core contract settle() function:

  function settle(
    uint256[] memory optionIds
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 amountOfWeth, uint256 rdpxAmount)
  {
    _whenNotPaused();
    (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
@>  ).settle(optionIds);                                   // @audit
    for (uint256 i = 0; i < optionIds.length; i++) {
      optionsOwned[optionIds[i]] = false;
    }

    reserveAsset[reservesIndex["WETH"]].tokenBalance += amountOfWeth;
    reserveAsset[reservesIndex["RDPX"]].tokenBalance -= rdpxAmount;

    emit LogSettle(optionIds);
  }

So, all option settlements can be prevented with a DOS of the subtractLoss(), function which will make all calls to settle() revert.

Coded Proof of Concept

This tests shows how the settle() functions becomes DOS, after 1 wei of collateral is sent to the vault LP.

Add this test to tests/rdpxV2-core/Unit.t.sol and run forge test --mt "testSettleDos":

  function testSettleDos() public {
    address _this = address(this); // track address(this) for later vm.prank

    address attacker = address(666);
    weth.transfer(attacker, 1); // give the attacker 1 wei of weth

    (,,,, address perpetualAtlanticVaultLp,,,,,) = rdpxV2Core.addresses();

    vault.addToContractWhitelist(address(rdpxV2Core));

    rdpxV2Core.bond(5 * 1e18, 0, address(this));
    rdpxPriceOracle.updateRdpxPrice(1e7);

    // The attacker can send 1 wei of the collateral at any point in time
    // This will make all calls to `settle()` revert
    vm.prank(attacker);
    weth.transfer(perpetualAtlanticVaultLp, 1); // remove this line to check that the test passes without it

    uint256[] memory _ids = new uint256[](1);
    _ids[0] = 0;

    // The `settle()` function will always revert and become in a DOS state
    vm.prank(_this);
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(_ids);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Replace the strict equality with a >=:

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

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L286-L295

Vulnerability details

Summary

The Uniswap addLiquidity() function expects the slippage params amountAMin, and amountBMin to be passed.

The reLP() sets those values as 0, which in other terms, means that the contract is ok with receiveing less amount of tokens than the fair market price when providing liquidity.

Impact

Less LP tokens will be received during the reLP() call, as it will accept any amount of tokens while adding liquidity to Uniswap.

Proof of Concept

The reLP() is used by the bond functions, and uses 0 for the amountAMin, and amountBMin values passed to the addLiquidity() function on the Uniswap router:

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,                    // @audit
      0,                    // @audit
      address(this),
      block.timestamp + 10
    );

• ReLPContract.sol#L286-L295
• addLiquidity() on Uniswap

This will make the function return less lp tokens than expected, while rebalancing.

Tools Used

Manual Review

Recommended Mitigation Steps

Set the amountAMin and amountBMin parameters to the expected minimum values

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L36-L44 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L122

Vulnerability details

Summary

The RdpxDecayingBonds contract keeps track of a bonds mapping with bonds information including the bond token owner. When the token is transfered, the bonds[bondId].owner value should be updated, but it isn't.

Impact

The owner value will be bricked when a token is transfered, as it can't be changed by any means.

Any integration relying on this value will make wrong trusted assumptions related to the bond token owner, potentially leading to critical issues as loss of funds, because of the importance of the owner attribute has.

Ranking it as Medium, as there is no direct loss of funds within the current scope, but bricks the contract functionality, as there is no way to fix it.

Proof of Concept

The owner attribute of the bonds mapping is set on each mint(), but its value is never updated:

  // Array of bonds
  mapping(uint256 => Bond) public bonds;

  // Structure to store the bond information
  struct Bond {
    address owner; // @audit
    uint256 expiry;
    uint256 rdpxAmount;
  }

RdpxDecayingBonds.sol#L36-L44

  function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) {
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
    uint256 bondId = _mintToken(to);
@>  bonds[bondId] = Bond(to, expiry, rdpxAmount); // @audit

    emit BondMinted(to, bondId, expiry, rdpxAmount);
  }

RdpxDecayingBonds.sol#L122

Coded Proof of Concept

This test shows how the owner remains the same on the bonds transfer after performing a transfer.

Add this test to the tests/RdpxDecayingBondsTest.t.sol file, and run forge test --mt "testSameOwnerOnTransfer":

  function testSameOwnerOnTransfer() public {
    // Mint a token
    rdpxDecayingBonds.mint(address(this), 1223322, 5e18);

    // The tokenId is 1
    // This can be confirmed by the fact that it changes its `ownerOf()` result on the OZ ERC721 after the transfer
    uint256 tokenId = 1;

    // Check the bond token owner before the transfer
    // Check for both via the `bonds` mapping, and the OZ `ownerOf()` function
    (address ownerBeforeTransfer,,) = rdpxDecayingBonds.bonds(tokenId);
    assertEq(ownerBeforeTransfer, address(this));
    assertEq(rdpxDecayingBonds.ownerOf(tokenId), address(this));

    // Transfer token
    rdpxDecayingBonds.safeTransferFrom(address(this), address(420), tokenId);

    // The `owner` changes on the OZ `ownerOf` function, while it remains the same on the `bonds` mapping
    (address ownerAfterTransfer,,) = rdpxDecayingBonds.bonds(tokenId);
    assertEq(ownerAfterTransfer, address(this));                        // @audit remains the same after the transfer
    assertEq(rdpxDecayingBonds.ownerOf(tokenId), address(420));
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Either update the owner value on each token transfer, or remove it, as the owner of the token is already tracked via the OZ ERC721 contract.

Assessed type

Token-Transfer

QA Report

Low Risk Findings

L-01 - Users can bond without providing WETH

RdpxV2Core::calculateBondCost() does not account for precission loss and allows users to bond an amount of 1, without providing any WETH.

• RdpxV2Core.sol#L904
• RdpxV2Core.sol#L705

Impact

Users can bond without 1 wei providing any WETH. This may break any assumptions regarding accountability of the Core contract, or may be combined with other issues to achieve a more critical one. This step may also be used in a for loop to increase the amount without providing WETH.

Proof of Concept

Add this test to tests/rdpxV2-core/Unit.t.sol and run forge test --mt "testBondNoEthCost":

  function testBondNoEthCost() public {
    uint256 initialWethBalance = weth.balanceOf(address(this));

    rdpxV2Core.bond(1, 0, address(this));

    uint256 finalWethBalance = weth.balanceOf(address(this));
    assertEq(initialWethBalance, finalWethBalance);
  }

Recommended Mitigation Steps

Validate that the returned wethRequired of the calculateBondCost() is greater than 0.

L-02 - decreaseAmount function name is misleading

RdpxDecayingBonds::decreaseAmount() does not decrease the bonds amount, but replaces its value. This is misleading and can lead to integration errors if they follow the Natspec: amount: amount to decrease.

  /// @param amount amount to decrease // @audit
  function decreaseAmount(
    uint256 bondId,
    uint256 amount
  ) public onlyRole(RDPXV2CORE_ROLE) {
    _whenNotPaused();
    bonds[bondId].rdpxAmount = amount; // @audit
  }

RdpxDecayingBonds.sol#L144

The only call to the function in the current scope is performed on the RdpxV2Core contract.

In this case the amount passed to the decreaseAmount() function is the expected decreased value, so the whole system works as expected.

    IRdpxDecayingBonds(addresses.rdpxDecayingBonds).decreaseAmount(
        _bondId,
        amount - _rdpxAmount
    );

RdpxV2Core.sol#L645

Nevertheless, this can lead to future errors.

Recommended Mitigation Steps

Change the name of the function to a better name like updateAmount(), or refactor the function and its calls to match its description.

L-03 - addAssetTotokenReserves() doesn't check that there is no token with the same symbol

The function checks that the token address is not used, but doesn't check for the symbol.

Different tokens may have the same symbol, which can be troublesome, as certain operations rely on the symbol.

• RdpxV2Core.sol#L240-L264

Recommended Mitigation Steps

• Verify that the token symbol is not previously used

L-04 - The removeAssetFromtokenReserves() removes tokens by symbol instead of per address

The removeAssetFromtokenReserves() function removes the first token it finds with a specified symbol, which may lead to an error, as multiple tokens can be added with the same symbol.

• RdpxV2Core.sol#L270-L290

Recommended Mitigation Steps

Remove tokens by their address instead of by their symbol.

L-05 - Approvals can't be completely revoked

The approveContractToSpend() function requires that the approved amount is > 0. This means that the function is not capable of revoke token approvals by setting them to 0.

This is also troublesome, as it is the expected behavior if a token approval wants to be removed. It can still be mitigated by sending a value of 1.

• UniV2LiquidityAmo.sol#L133
• RdpxV2Core.sol#L410

Recommended Mitigation Steps

Remove the unnecessary requirement of amount > 0.

L-06 - provideFunding() may be victim to a DOS because of strict equality

PerpetualAtlanticVault::payFunding() has an strict equality, which will revert if totalActiveOptions is different than fundingPaymentsAccountedFor[latestFundingPaymentPointer].

  function payFunding() external onlyRole(RDPXV2CORE_ROLE) returns (uint256) {
    _whenNotPaused();
    _isEligibleSender();

    _validate(
@>    totalActiveOptions ==                                       // @audit
        fundingPaymentsAccountedFor[latestFundingPaymentPointer],
      6
    );

PerpetualAtlanticVault.sol#L372-L380

• Only fundingPaymentsAccountedFor[] is increased in the calculateFunding() function.
• Only totalActiveOptions is decreased in the settle() function.
• Both attributes are increased with the same amount in the purchase() function.

payFunding() is called by RdpxV2Core::provideFunding(), which will make it revert if than strict equality may be broken.

Recommended Mitigation Steps

Change the strict equality to a comparison operator.

Non-Critical Findings

NC-01 - Use camelCase for function names

Functions as addAssetTotokenReserves() are not correctly formatted as camelCase presumably from the fact that they modify the tokenReserves variable.

Nevertheless they should use camelCase for consistency with the rest of the codebase

• RdpxV2Core.sol#L240
• RdpxV2Core.sol#L270
• ReLPContract.sol#L90

Recommended Mitigation Steps

Replace the function names with the camelCase version as addAssetToTokenReserves() for example.

NC-02 - Unnecessary role check

Role is checked both on the modifier and the function body. Consider removing one:

  function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) { // @audit
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter"); // @audit

RdpxDecayingBonds.sol#L114-L120

NC-03 - RDPXV2CORE_ROLE is not assigned in PerpetualAtlanticVault

While the DEFAULT_ADMIN_ROLE, and MANAGER_ROLE are assigned during the constructor(), the RDPXV2CORE_ROLE is not.

Any call from the Core contract to the vault will revert until this value is set, so it would be advised to assign this role in the constructor as well.

PerpetualAtlanticVault.sol#L113-L128

NC-03 - Unnecessary struct attribute prefix

TokenAInfo attributes are all prefixed with tokenA. This is unnecessary, as they are known to be from a TokenAInfo struct. Consider removing the prefix:

  struct TokenAInfo {
    // tokenA reserves
    uint256 tokenAReserve;
    // rdpx price
    uint256 tokenAPrice;
    // tokenA LP reserves
    uint256 tokenALpReserve;
  }

ReLPContract.sol#L51-L58

NC-04 - Repeated expression

Long expression that are repeated can lead to errors if refactored, and are harder to read.

Consider creating a variable to store the expression (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) / 1e18, which is repeated 3 times:

  collateralToken.safeTransfer(
    addresses.perpetualAtlanticVaultLP,
    (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) / // @audit
      1e18
  );

  IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
    .addProceeds(
      (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) / // @audit
        1e18
    );

  emit FundingPaid(
    msg.sender,
    ((currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) / // @audit
      1e18),
    latestFundingPaymentPointer
  );