Dopex - Viktor_Cortess's results

Lines of code

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

Vulnerability details

Impact

Incorrect accounting of WETH leads to incorrect balances after withdrawal.

Proof of Concept

When a user delegates WETH the amount delegated is added to the variable totalWethDelegated:

function addToDelegate(
uint256 _amount,
uint256 _fee
  ) external returns (uint256) {
_whenNotPaused();
// fee less than 100%
_validate(_fee < 100e8, 8);
// amount greater than 0.01 WETH
_validate(_amount > 1e16, 4);
// fee greater than 1%
_validate(_fee >= 1e8, 8);

IERC20WithBurn(weth).safeTransferFrom(msg.sender, address(this), _amount);

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

}

However, when the user withdraws the delegated funds, the withdrawn amount is not subtracted from totalWethDelegated:

unction 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);

}

Example of an attack:

Bob knows that the Core contract holds a balance of 10 WETH. He takes a flash loan of 20 WETH, adds it to the delegate, then withdraws his 20 WETH. Now, the contract holds a balance of 10 WETH, but totalWethDelegated is still at 20 WETH.

If someone then calls the sync() function, it will result in an underflow because balance < totalWethDelegated:

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; //underflow
  }
  reserveAsset[i].tokenBalance = balance;
}

emit LogSync();

}

Tools Used

vs

Recommended Mitigation Steps

To mitigate this vulnerability, subtract the withdrawn amount from totalWethDelegated in the withdraw function, like so:

totalWethDelegated -= amountWithdrawn; to the withdraw function.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L286-L295

Vulnerability details

Impact

Executing the addLiquidity function from UniswapV2 with zero-valued amountAMin and amountBMin could result in a loss of funds due to MEV bot attacks.

Proof of Concept

amountAMin and amountBMin are the minimum amounts of tokenA and tokenB you are willing to add (acts as slippage protection).

286: (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
  addresses.tokenA,
  addresses.tokenB,
  tokenAAmountOut,
  amountB / 2,
  0,//@Mid zero
  0,
  address(this),
  block.timestamp + 10
);

Setting amountAMin and amountBMin to zero removes a layer of protection against slippage, potentially exposing the user to risks like MEV (Miner Extractable Value) attacks, where a miner could manipulate the transaction in a way that is disadvantageous to the user.

Tools Used

vs

Recommended Mitigation Steps

Consider adding a function that calculates minimal amounts.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178

Vulnerability details

Impact

Two identical functions exist in the UniV2LiquidityAMO and UniV3LiquidityAMO contracts, but only function from UniV3LiquidityAMO synchronizes balances of the Core contract after sending tokens to it.

Proof of Concept

Function _sendTokensToRdpxV2Core from UniV2LiquidityAMO :

function _sendTokensToRdpxV2Core() internal {
uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf( 
  address(this)
);
uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
  address(this)
);
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(addresses.tokenA).safeTransfer(
  addresses.rdpxV2Core,
  tokenABalance
);
IERC20WithBurn(addresses.tokenB).safeTransfer(
  addresses.rdpxV2Core,
  tokenBBalance
);

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

}

Function _sendTokensToRdpxV2Core from UniV3LiquidityAMO :

 function _sendTokensToRdpxV2Core(address tokenA, address tokenB) internal {
uint256 tokenABalance = IERC20WithBurn(tokenA).balanceOf(address(this));
uint256 tokenBBalance = IERC20WithBurn(tokenB).balanceOf(address(this));
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(tokenA).safeTransfer(rdpxV2Core, tokenABalance);
IERC20WithBurn(tokenB).safeTransfer(rdpxV2Core, tokenBBalance);

// sync token balances
IRdpxV2Core(rdpxV2Core).sync();

emit LogAssetsTransfered(tokenABalance, tokenBBalance, tokenA, tokenB);

}

The lack of the call of this function causes discrepancies between the reserve balance of reserve tokens and totalWETHdelegated in the Core contract.

Tools Used

vs

Recommended Mitigation Steps

Adding IRdpxV2Core(rdpxV2Core).sync(); to the _sendTokensToRdpxV2Core in the _sendTokensToRdpxV2Core contract.

Assessed type

Context