Dopex - Jorgect's results

Lines of code

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

Vulnerability details

The withdraw function is missing subtract the amount of the delegatePosition in totalWethDelegated, this can allow a user or a attacker block all the weth reserve.

Impact

A user can block all the weth reserve in the RpdxV2Core.sol, letting the contract unusuable lossing the weth reserve for ever.

Proof of Concept

The totalWethDelegated is a variable that track the total amount of weth that the protocol has available to use in bondWithDelegate function.

let´s check the addDelegate function:

file:contracts/core/RdpxV2Core.sol
function addToDelegate(
    uint256 _amount,
    uint256 _fee
  ) external returns (uint256) {
    ...

    Delegate memory delegatePosition = Delegate({
      amount: _amount,
      fee: _fee,
      owner: msg.sender,
      activeCollateral: 0
    });
    delegates.push(delegatePosition);

    // add amount to total weth delegated
    totalWethDelegated += _amount;
    ...
  }

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

the addToDelegate function is increasing the totalWethDelegated. Now when a user bond With Delegate the amount of weth delegated are substracted in totalWethDelegated variable:

file:contracts/core/RdpxV2Core.sol
  function _bondWithDelegate(
    uint256 _amount,
    uint256 rdpxBondId,
    uint256 delegateId
  ) internal returns (BondWithDelegateReturnValue memory returnValues) { 
    ...
    totalWethDelegated -= wethRequired;
    ...
  }

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

if the delegatePosition still has some weth that are not been using, the owner of the delegatePosition can withdraw them

file:contracts/core/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;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

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

We can see perfectly that this amount of weth are no substracted in the totalWethDelegated variable.

This is problematic due the contract are performing important calculation with this variable

see the above proof of concept (this test have to be run it in Unit.t.sol in rdpxV2-core test folder):

function testIncorrectWhitdrawT() public {
        //Delegate son WETH
        
        uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);

        //First check of the totalWethDelegate
        uint256 first_totalwethdelegate = rdpxV2Core.totalWethDelegated();
        console.log("first check of the totalWethDelegate:", first_totalwethdelegate);

        rdpxV2Core.withdraw(delegateId);
        uint256 second_totalwethdelegate = rdpxV2Core.totalWethDelegated();
        console.log("second check of the totalWethDelegate:", second_totalwethdelegate);

        assert(second_totalwethdelegate == 0);
    }

This assertion have to be passed but it doest´n and if we check the logs

Logs:
  first check of the totalWethDelegate: 50000000000000000000
  second check of the totalWethDelegate: 50000000000000000000

The totalWethDelegate is never reduced.

At this point this is problematic but it could be more problematic if a user call the sync function:

file:contracts/core/RdpxV2Core.sol
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) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;
    }

    emit LogSync();
  }

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

This function is syncronizing the weth balance of the contract minus the totalWethDelegated but remenber that this totalWethDelegated is actually more than it should be due the problem in the withdraw functions.

So if an attacker follow the next path can block the weth funds of the contract:

1. Calculate the amount of the resever of the contract.
2. take a flashloan.
3. Create a delegatePosition with the amount of the reserve.
4. Withdraw this position.
5. Call sync function.
6. Repay the flashloan.
7. The weth resever is done

Tools Used

Manual, foundry

Recommended Mitigation Steps

Substract the totalWethDelegated in when a user withdraw a delegatePosition.

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 -= delegate.amount;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Error