Dopex - qbs's results

Lines of code

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

Vulnerability details

A delegate bonding system is used by users who can delegate their WETH tokens, set a fee for the usage of these tokens, and other users holding rDPX can then bond using these pooled funds. The withdraw function allows a user who previously added WETH to the delegation pool via addToDelegate to withdraw any WETH that hasn't been actively used (bonded) by others.

However, when users retrieve their unused WETH from the delegate pool, the global state variable totalWethDelegated is not adjusted to reflect this withdrawal.

Impact

Since the totalWethDelegated is not reduced when a delegate withdraws, the contract falsely believes there's more WETH available for delegation than there actually is. This can cause potential errors in subsequent bonding operations.

If the discrepancy between the contract's internal account (totalWethDelegated) and the actual amount of WETH delegated grows large, the sync function would always revert. This could happen if a malicious user repeatedly calls addToDelegate and withdraw, causing a potentially large value in totalWethDelegated to be subtracted from the actual balance. This subtraction might lead to a negative value, causing a revert and effectively disrupting reLP and AMO operations.

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }

Proof of Concept

In the addToDelegate function, users deposit WETH and set a fee. The totalWethDelegated variable is incremented by the deposited _amount.

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

The user who had previously invoked addToDelegate has the ability to withdraw unused WETH via the withdraw function. However, during this withdrawal operation, there's a notable omission: the totalWethDelegated variable isn't decremented by the amountWithdrawn.

Tools Used

Manual review

Recommended Mitigation Steps

Subtract the amountWithdrawn from totalWethDelegated in the withdraw function.

Assessed type

Other

Lines of code

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

Vulnerability details

The UniV2LiquidityAMO contract includes addLiquidity, removeLiquidity, and swap methods that are called by reLPContract (which subsequently update the rdpxV2Core balances after the call), but also, according to the sponsor, are intended to be invoked directly by privileged actor. Each of these methods eventually ends up transferring assets to the rdpxV2Core contract using the _sendTokensToRdpxV2Core function. However, upon transferring these assets, the said function does not invoke the sync method of rdpxV2Core to update the internal accounting balances. This can lead to accounting discrepancies and potential imbalances between expected and actual assets in rdpxV2Core.

Impact

This oversight can introduce accounting discrepancies. Tokens that are transferred to the rdpxV2Core contract via the _sendTokensToRdpxV2Core function will update the token balances in the rdpxV2Core contract. However, without calling sync, the reserveAsset accounting structure within rdpxV2Core will not reflect the updated token balances. Consequently, some functions subtracting value from reserveAsset might revert due to underflow and off-chain AMO mechanisms that rely on this internal balance may not work as intended.

Proof of Concept

1. The addLiquidity, removeLiquidity, and swap methods in UniV2LiquidityAMO each have a step that involves sending tokens to the rdpxV2Core contract.
2. The _sendTokensToRdpxV2Core function transfers the required token amounts but does not inform the rdpxV2Core contract about the changes.
3. The sync method in rdpxV2Core exists specifically to update internal accounting based on the actual token balances. Without a call to sync post token transfer, the rdpxV2Core won't have updated accounting values, leading to a discrepancy between recorded and actual balances.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the _sendTokensToRdpxV2Core function in the UniV2LiquidityAMO contract to include a call to the sync method of rdpxV2Core post the token transfers - in the same way as it is performed in UniV3LiquidityAMO.

Assessed type

Other