code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/amo/UniV3LiquidityAmo.sol#L361 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1185 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1110 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1106 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/reLP/ReLPContract.sol#L306 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L780 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L803

Vulnerability details

Impact

The Sync function plays a critical role in overall wellbeing of the protocol by syncing the backing reserves within the Core contract. It is called by the Uniswap AMOS for instance whenever they want to add liquidity into the backing reserve, or by the ReLP contract whenever it wants to send rdpx to the backing reserve.

As mentioned previously, the sync function is extremely important, as it is responsible for updating the reserve asset mapping to reflect the liquidity of the backing reserve.

However a user who delegates eth can either unintentionally or maliciously permanently DOS the ability to call this function. This can be done by manipulating the value of the totalWethDelegated variable.

  function sync() external {
// @audit syncs Backing reserves with contract balances
for (uint256 i = 1; i < reserveAsset.length; i++) {
  uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
    address(this)
  );

  if (weth == reserveAsset[i].tokenAddress) {
    // @audit if totalWethDelegated is inflated, this function will never
    // work, complete DOS because line below will cause a revert with underflow
    balance = balance - totalWethDelegated;
  }
  reserveAsset[i].tokenBalance = balance;
}

emit LogSync();

}

The user can increase the value of totalWethDelegated by calling the addToDelegate. The user can then call withdraw which allows him to withdraw unused WETH that he delegated. the oversight is that the totalWethDelegated variable is not updated to handle this withdrawal, ergo the balance of WETH in contract will be reduced by the amount of withdrawn while totalWethDelegated will still be inflated causing the calculation to update balance of weth in sync function to revert due to underflow.

The consequence of this is obvious, the sync function will be DOS'ed. The impact is quite severe since AMOS will unable to provide liquidity to the core contract. RELP functionality will also be bricked. in addition, if the sync function wont work, then internal operations within the core contract will be affected. for instance the lowerDepeg function responsible for ensuring the peg dpxETH to ETH on the curve pool is maintained could also be compromised if the rdpx tokenbalance in the reserverAsset mapping can not be properly updated via a sync operation, leading to an underflow if _rdpxAmount amount to deduct is larger than the RDPX token balance.

  // @audit this line in the lowerDepeg might fail if the tokenBalance was not updated via the syn function
  reserveAsset[reservesIndex["RDPX"]].tokenBalance -= _rdpxAmount;

Proof of Concept

    function testManipulateWethBalance() public {
/// add to delegate with different fees
uint256 userBalance = weth.balanceOf(address(this));
uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);
uint256 delgateId2 = rdpxV2Core.addToDelegate(10 * 1e18, 20 * 1e8);
uint256 delgateId3 = rdpxV2Core.addToDelegate(10 * 1e18, 30 * 1e8);

// assert that the user balance is reduced by the amount
assertEq(weth.balanceOf(address(this)), userBalance - 70 * 1e18);
assertEq(rdpxV2Core.totalWethDelegated(), 70 * 1e18);

rdpxV2Core.withdraw(delegateId);
rdpxV2Core.withdraw(delgateId2);
rdpxV2Core.withdraw(delgateId3);

// assert that the user balance is back to origin
assertEq(weth.balanceOf(address(this)), userBalance);
// assert total weth delegated variable is still the same..
assertEq(rdpxV2Core.totalWethDelegated(), 70 * 1e18);

vm.expectRevert(stdError.arithmeticError);

rdpxV2Core.sync();

}

The POC above clearly highlights this attack. in a single transaction, a malicious user for instance can delegate and then undelegate leaving the totalWethDelegated variabled inflated. the call to sync will fail with arithmetic underflow error. The malicious user can for instance take out a flash loan for a large amount of ETH, and use that to severely inflate the totalWethDelegated. they can within the same transaction immediately withdraw the delegated ETH.

Tools Used

Manual Review

Recommended Mitigation Steps

The Fix is obvious, make sure the totalWethDelegated is updated properly to reflect the current balance after a user withdraws any delegated ETH.

Assessed type

DoS