code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L941-L968 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L995-L1008

Vulnerability details

Impact

The contract's WETH amount gets permanently bricked.

Proof of Concept

A user can call addToDelegate() and give WETH, that other people can use for bonding with their rDPX in exchange for a certain percentage appointed by the delegatee.

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

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

    emit LogAddToDelegate(_amount, _fee, delegates.length - 1);
    return (delegates.length - 1);

There the user gets added to the delegates array and also the delegated WETH amount gets added to a variable called totalWethDelegated, which is used for keeping track of the part of WETH in the contract, which is owned by delegates. That variable is also used in sync() for setting the virtual balance of WETH in the contract.

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();
 }

The issue arises due to the WETH amount not being removed from totalWethDelegated upon withdrawal.

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;

        // @audit-issue The delegate entry's amount gets removed, but the amount doesn't get subtracted from totalWethDelegated

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Consider the following PoC demonstrating the issue:

https://gist.github.com/CrisCodesCrap/fb5ad3b5a5c95670d2ae44c895b42ab5

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Consider removing the amount upon withdrawing the delegated WETH from the protocol.

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 -= amountWithdrawn;
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Other

c4-pre-sort commented 1 year ago

bytes032 marked the issue as high quality report

c4-pre-sort commented 1 year ago

bytes032 marked the issue as primary issue

bytes032 commented 1 year ago

Valid issue. One thing that the report is missing is the potential for DoS through the sync() function.

bytes032 commented 12 months ago

It has various different nuances:

Breaks the accounting (the amount of WETH deposited for delegated bonding)
Unintentionally DoS multiple functions, for example sync() here:

Can be used by “malicious” actors for intentional dos, e.g. with FlashLoan [1]
Note that it will also break provideFunding() and lowerDepeg()

Note that even if WETH is donated to resolve the block, the accounting of WETH reserves will still be completely wrong

c4-sponsor commented 11 months ago

psytama (sponsor) confirmed