Dopex - dethera's results

Lines of code

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

Vulnerability details

Impact

The state variable totalWethDelegated does not track correctly the total amount of WETH delegated, because it is not updated in the withdraw function.

Proof of Concept

In the file RdpxV2Core.sol there is a function withdraw, which lets the caller withdraw the unused collateral from a delegatePosition he created. The unused collateral is calculated as:

amountWithdrawn = delegate.amount - delegate.activeCollateral;

After that the amountWithdrawn is sent to the caller:

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

Since the caller takes amountWithdrawn back, the total amount of WETH delegated is decreased by amountWithdrawn and the state variable tracking the total amount of WETH delegated should be updated like so:

totalWethDelegated -= amountWithdrawn;

However, the withdraw function is missing the line:

totalWethDelegated -= amountWithdrawn;

which leads to a bug. As a result, the state variable totalWethDelegated does not show correctly the amount of WETH that is delegated.

Tools Used

Manual review

Recommended Mitigation Steps

Add the following line to the withdraw function:

totalWethDelegated -= amountWithdrawn;

After the withdraw function is fixed, it should look like:

  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

Error

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/decaying-bonds/RdpxDecayingBonds.sol#L135-L145 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/decaying-bonds/RdpxDecayingBonds.sol#L144 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L643-L646

Vulnerability details

Impact

Potentially, the impact can be catastrophic - with the current implementation, the caller of the function decreaseAmount may unintentionally increase the bond amount thinking the bond amount is decreased.

Proof of Concept

In the file RdpxDecayingBonds.sol there is a function decreaseAmount, which is expected to decrease the bond amount of a specific bond. However, the logic that 'decreases' the amount is implemented as:

bonds[bondId].rdpxAmount = amount;

There are no checks in place to verify that bonds[bondId].rdpxAmount > amount. This implementation puts severe burden on the caller of the function decreaseAmount - the caller should always know what the value of bonds[bondId].rdpxAmount is in order to specify amount in such a way that bonds[bondId].rdpxAmount > amount, which is insanely error-prone. The caller of the function decreaseAmount, not aware what the value of bonds[bondId].rdpxAmount is, may call the function with amount such that bonds[bondId].rdpxAmount < amount, the line:

bonds[bondId].rdpxAmount = amount;

will execute. As a result of the call to the function decreaseAmount, the value of bonds[bondId].rdpxAmount increases.

Tools Used

Manual review

Recommended Mitigation Steps

The function can be fixed in several ways:

1. Add a require check at the beginning of the function to make sure that bonds[bondId].rdpxAmount > amount. This will make sure that after every successful call to decreaseAmount the value of bonds[bondId].rdpxAmount will truly decrease.

  function decreaseAmount(
    uint256 bondId,
    uint256 amount
  ) public onlyRole(RDPXV2CORE_ROLE) {
    _whenNotPaused();
    require(bonds[bondId].rdpxAmount > amount, "Error, amount should be less than bonds[bondId].rdpxAmount");
    bonds[bondId].rdpxAmount = amount;
  }

2. Another way of fixing the function is to define amount as the amount that will be subtracted from bonds[bondId].rdpxAmount, thus decreasing the value of bonds[bondId].rdpxAmount.

  function decreaseAmount(
    uint256 bondId,
    uint256 amount
  ) public onlyRole(RDPXV2CORE_ROLE) {
    _whenNotPaused();
    bonds[bondId].rdpxAmount -= amount;
  }

Assessed type

Invalid Validation