088.md

Powerful Honeysuckle Anteater

High

Cross-contract Reentrancy between Borrowing and CDS contracts will leave the omnichain in wrong state

Summary

During a dCDS withdrawal, raw ETH is transferred before updating global state variables. A malicious user can exploit this through cross-contract reentrancy, re-entering the borrowing contract and performing operations with an incorrect state, which is then broadcasted to the omni-chain. This disrupts key storage fields and can result in asset loss.

Root Cause

The root cause is that during a dCDS withdrawal, raw ETH is transferred to the dCDS user (opted-in for liquidations) before updating the omni-chain data and other fields, leaving the state unfinished. Reference: CDSLib.sol#L825-L831

    function withdrawUser(......) public returns (CDSInterface.WithdrawResult memory) {
.....Skipping Code.....
               if (params.ethAmount != 0) {
                    params.omniChainData.collateralProfitsOfLiquidators -= totalWithdrawCollateralAmountInETH;
                    // Call transferEthToCdsLiquidators to tranfer eth
                    //@audit-issue reentrancy is possible here
@>>                  interfaces.treasury.transferEthToCdsLiquidators(msg.sender, params.ethAmount);
                }
                if (weETHAmount != 0) {
                    interfaces.treasury.approveTokens(IBorrowing.AssetName.WeETH, address(interfaces.cds), weETHAmount);
                    bool sent = IERC20(interfaces.borrowing.assetAddress(IBorrowing.AssetName.WeETH)).transferFrom(
                        address(interfaces.treasury), msg.sender, weETHAmount
                    );
                    if (!sent) revert CDSInterface.CDS_TransferFailed(IBorrowing.AssetName.WeETH);
                }
                if (rsETHAmount != 0) {
                    interfaces.treasury.approveTokens(IBorrowing.AssetName.WrsETH, address(interfaces.cds), rsETHAmount);
                    bool sent = IERC20(interfaces.borrowing.assetAddress(IBorrowing.AssetName.WrsETH)).transferFrom(
                        address(interfaces.treasury), msg.sender, rsETHAmount
                    );
                    if (!sent) revert CDSInterface.CDS_TransferFailed(IBorrowing.AssetName.WrsETH);
                }
            }

            return CDSInterface.WithdrawResult(.......);
        }
    }

And in transferEthToCdsLiquidators() there is an .call which allows any user to re-enter into a different arbitrary function call:

    function transferEthToCdsLiquidators(address user, uint128 amount) external onlyCoreContracts {
        require(user != address(0) && amount != 0, "Input address or amount is invalid");
        // Get the omnichain data
        IGlobalVariables.OmniChainData memory omniChainData = globalVariables.getOmniChainData();
        // Check whether treasury has enough collateral to transfer
        require(amount <= omniChainData.collateralProfitsOfLiquidators, "Treasury don't have enough ETH amount");
        omniChainData.collateralProfitsOfLiquidators -= amount;
        globalVariables.setOmniChainData(omniChainData);

        // Transfer ETH to user
@>>     (bool sent,) = payable(user).call{value: amount}("");
        if (!sent) revert Treasury_EthTransferToCdsLiquidatorFailed();
    }

However in the CDSLib.sol withdrawUser function we aren't updating some global variables, which allow the re-entrancy to have impact. Reference in CDS.sol

    function withdraw(....) external payable nonReentrant whenNotPaused(IMultiSign.Functions(5)) {
    ......Skipping Code......

        WithdrawResult memory withdrawResult;
@>>     withdrawResult = CDSLib.withdrawUser(
            WithdrawUserParams(
                cdsDepositDetails,
                omniChainData,
                optionFees,
                optionsFeesToGetFromOtherChain,
                returnAmount,
                withdrawResult.ethAmount,
                withdrawResult.usdaToTransfer,
                weETH_ExchangeRate,
                rsETH_ExchangeRate,
                fee.nativeFee
            ),
            Interfaces(
                treasury,
                globalVariables,
                usda,
                usdt,
                borrowing,
                CDSInterface(address(this))
            ),
            totalCdsDepositedAmount,
            totalCdsDepositedAmountWithOptionFees,
            omniChainCDSLiqIndexToInfo
        );

@>>     totalCdsDepositedAmount = withdrawResult.totalCdsDepositedAmount;
@>>     totalCdsDepositedAmountWithOptionFees = withdrawResult.totalCdsDepositedAmountWithOptionFees;
@>>     withdrawResult.omniChainData.cdsPoolValue -= cdsDepositDetails.depositedAmount;
        // Update the user deposit data
@>>     cdsDetails[msg.sender].cdsAccountDetails[index] = withdrawResult.cdsDepositDetails;
        // Update the global omnichain struct
@>>     globalVariables.setOmniChainData(withdrawResult.omniChainData);
        // Check whether after withdraw cds have enough funds to protect borrower's collateral
        if (withdrawResult.omniChainData.totalVolumeOfBorrowersAmountinWei != 0) {
            if (borrowing.calculateRatio(0, ethPrice) < (2 * CDSLib.RATIO_PRECISION)) revert CDS_NotEnoughFundInCDS();
        }

        if (ethPrice != lastEthPrice) {
@>>       updateLastEthPrice(ethPrice);
        }

.......
    }

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. User A sends a transaction to withdraw their dCDS deposit.
2. When the code reaches the transferEthToCdsLiquidators() function, User A re-enters the borrowers.sol contract to initiate a withdrawal.
3. The withdrawal is successful, but the global variables updated during the borrower withdrawal are submitted through LayerZero before completing the dCDS withdrawal operation. As a result, two operations are performed with the same initial state.

Impact

At a minimum, this could lead to incorrect tracking of total assets or depositors if the totalBorrowAmounts and other fields changed during the borrower withdrawal are overridden by the dCDS withdrawal's global variable updates. The omniChainData set by dCDS after re-entering and completing the withdrawal does not account for the borrower withdrawal state when using setOmniChainData to update them.

This can lead to double-counting of the cumulativeValue, as the lastETHPrice in dCDS is not updated, and we call updateCumulativeValueInCDS() in BorrowingLib.sol during withdrawal. Other state changes may also not be reflected accurately.

Ultimately, this could result in significant discrepancies, causing the protocol to overestimate its resources. This overestimation could prevent other users from successfully withdrawing, effectively bricking the protocol.

Mitigation

Apply the CEI (Check-Effects-Interactions) pattern. Ensure that ETH transfers (or any transfers that invoke callbacks to the receiver) occur only after all state changes have been completed.