LeverageMacroBase::_doOperation will revert if willSweep is set to true

[c4-bot-6]

Lines of code

https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L241-L243
https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L248
https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L43-L46

Vulnerability details

LeverageMacroBase.sol has an internal function _doOperation that handles the execution of the CDP operation requested by an user, which can be opening a CDP, adjusting a CDP or closing a CDP.

LeverageMacroBase::_doOperation

function _doOperation(
        FlashLoanType flType,
        uint256 borrowAmount,
        LeverageMacroOperation memory operation,
        PostOperationCheck postCheckType,
        PostCheckParams memory checkParams,
        bytes32 expectedCdpId
    ) internal {
        // Call FL Here, then the stuff below needs to happen inside the FL
        if (operation.amountToTransferIn > 0) {
            IERC20(operation.tokenToTransferIn).safeTransferFrom(
                msg.sender,
                address(this),
                operation.amountToTransferIn
            );
        }

        // Take eBTC or stETH FlashLoan
        if (flType == FlashLoanType.eBTC) {
            IERC3156FlashLender(address(borrowerOperations)).flashLoan(
                IERC3156FlashBorrower(address(this)),
                address(ebtcToken),
                borrowAmount,
                abi.encode(operation)
            );
        } else if (flType == FlashLoanType.stETH) {
            IERC3156FlashLender(address(activePool)).flashLoan(
                IERC3156FlashBorrower(address(this)),
                address(stETH),
                borrowAmount,
                abi.encode(operation)
            );
        } else {
            // No leverage, just do the operation
            _handleOperation(operation);
        }

After handling the operation, the _doOperation function performs post-operation checks to ensure that the CDP position values, such as debt and collateral, are as expected post-operation execution.

				/**
         * POST CALL CHECK FOR CREATION
         */
        if (postCheckType == PostOperationCheck.openCdp) {
            // Check for param details
            ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(expectedCdpId);
            _doCheckValueType(checkParams.expectedDebt, cdpInfo.debt);
            _doCheckValueType(checkParams.expectedCollateral, cdpInfo.coll);
            require(
                cdpInfo.status == checkParams.expectedStatus,
                "!LeverageMacroReference: openCDP status check"
            );
        }

        // Update CDP, Ensure the stats are as intended
        if (postCheckType == PostOperationCheck.cdpStats) {
            ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(checkParams.cdpId);

            _doCheckValueType(checkParams.expectedDebt, cdpInfo.debt);
            _doCheckValueType(checkParams.expectedCollateral, cdpInfo.coll);
            require(
                cdpInfo.status == checkParams.expectedStatus,
                "!LeverageMacroReference: adjustCDP status check"
            );
        }

        // Post check type: Close, ensure it has the status we want
        if (postCheckType == PostOperationCheck.isClosed) {
            ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(checkParams.cdpId);

            require(
                cdpInfo.status == checkParams.expectedStatus,
                "!LeverageMacroReference: closeCDP status check"
            );
        }

After performing the post-operation checks, there is a selection statement that checks if willSweep has been set to true and if it evaluates to true, the _doOperation function calls sweepToCaller to ensure that any dust that has been left over from the performed operation is transferred back to the user.

  // Sweep here if it's Reference, do not if it's delegate
        if (willSweep) {
@>          sweepToCaller();
        }

The sweepToCaller function calls another internal function _assertOwner, which checks who is the caller of the sweepToCaller function and compares it against the owner of the LeverageZapRouterBase contract that has been initialized during the contract construction.

LeverageMacroBase::sweepToCaller

/// @notice Sweep away tokens if they are stuck here
function sweepToCaller() public {
@>  _assertOwner();
    /**
     * SWEEP TO CALLER *
     */
    // Safe unchecked because known tokens
    uint256 ebtcBal = ebtcToken.balanceOf(address(this));
    uint256 collateralBal = stETH.sharesOf(address(this));

    if (ebtcBal > 0) {
        ebtcToken.transfer(msg.sender, ebtcBal);
    }

    if (collateralBal > 0) {
        stETH.transferShares(msg.sender, collateralBal);
    }
}

LeverageMacroBase::_assertOwner

function _assertOwner() internal {
    // Reference will compare to variable,
@>  require(owner() == msg.sender, "Must be owner");
}

LeverageZapRouterBase::constructor

abstract contract LeverageZapRouterBase is ZapRouterBase, LeverageMacroBase, ReentrancyGuard, IEbtcLeverageZapRouter {
    using SafeERC20 for IERC20;

    uint256 internal constant PRECISION = 1e18; 
    uint256 internal constant BPS = 10000; 

    address public immutable theOwner;
    address public immutable DEX;
    uint256 public immutable zapFeeBPS;
    address public immutable zapFeeReceiver;

    constructor(
        IEbtcLeverageZapRouter.DeploymentParams memory params
    )
        ZapRouterBase(
            params.borrowerOperations, 
            IERC20(params.wstEth), 
            IERC20(params.weth), 
            IStETH(params.stEth)
        )
        LeverageMacroBase(
            params.borrowerOperations,
            params.activePool,
            params.cdpManager,
            params.ebtc,
            params.stEth,
            params.sortedCdps,
            false // Do not sweep 
        )
    {
        if (params.zapFeeBPS > 0) { 
            require(params.zapFeeReceiver != address(0));
        }

@>      theOwner = params.owner; 
        DEX = params.dex;
        zapFeeBPS = params.zapFeeBPS;
        zapFeeReceiver = params.zapFeeReceiver;

        // Infinite Approvals @TODO: do these stay at max for each token?
        ebtcToken.approve(address(borrowerOperations), type(uint256).max);
        stETH.approve(address(borrowerOperations), type(uint256).max);
        stETH.approve(address(activePool), type(uint256).max);
        stEth.approve(address(wstEth), type(uint256).max); 
    }

However, given that any user can perform operations such as opening a CDP, adjusting a CDP and closing a CDP, it is unsafe to assume that the caller of the sweepToCaller function will be equal to the owner address initialized during the LeverageZapRouterBase construction.

Impact

If willSweep is set to true, all main functions - openCdp, adjustCdp, closeCdp - will revert because msg.sender can be any arbitrary EOA address and would not necessarily be equal to the owner address of the contract.

Although in the specific case of LeverageZapRouterBase, the willSweep variable is initialized to false, the overall implementation is flawed. The sweepToCaller function can only be called by the owner of the LeverageZapRouterBase and no one else, effectively making it a sweepToOwner instead of a sweepToCaller.

Proof of Concept

https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L241-L243

https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L248

https://github.com/ebtc-protocol/ebtc/blob/2da05f2d1354190eacf64ec256d2c5c5358e5fa2/packages/contracts/contracts/LeverageMacroBase.sol#L43-L46

Tools Used

Manual Review

Recommended Mitigation Steps

Change the sweepToCaller function's visibility to private to ensure it cannot be called by anyone other than the contract itself and remove the _assertOwner call, so that it can properly transfer the remaining dust to any arbitrary EOA address and not just the owner address.

    /// @notice Sweep away tokens if they are stuck here
-    function sweepToCaller() public {
+    function sweepToCaller() private {
-       _assertOwner();
        /**
         * SWEEP TO CALLER *
         */
        // Safe unchecked because known tokens
        uint256 ebtcBal = ebtcToken.balanceOf(address(this));
        uint256 collateralBal = stETH.sharesOf(address(this));

        if (ebtcBal > 0) {
            ebtcToken.transfer(msg.sender, ebtcBal);
        }

        if (collateralBal > 0) {
            stETH.transferShares(msg.sender, collateralBal);
        }
    }

Assessed type

DoS