Badger eBTC Audit + Certora Formal Verification Competition - Stormy's results

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/LeverageMacroBase.sol#L398-L431

Vulnerability details

Impact

On a short explanation, leverage macros are contracts specifically made to interact with the eBTC system, with a leverage macro you can:

• Open, adjust and close Cdps.
• Take eBTC, stETH flashloans
• Make swaps in DEXes
• Do arbitrary calls

Everyone is free to make a call to one of the functions in LeverageMacroFactory and deploy a new copy of the reference implementation of LeverageMacro for self use.

    /// @notice Deploys a new macro for you
    function deployNewMacro() external returns (address) {
        return deployNewMacro(msg.sender);
    }

    /// @notice Deploys a new macro for an owner, only they can operate the macro
    function deployNewMacro(address _owner) public returns (address) {
        address addy = address(
            new LeverageMacroReference(
                borrowerOperations,
                activePool,
                cdpManager,
                ebtcToken,
                stETH,
                sortedCdps,
                _owner
            )
        );

        emit DeployNewMacro(_owner, addy);

        return addy;
    }

While having an opened Cdp position, there is always a chance for your position to be fully redeemed or liquidated. In this two cases there will always be a surplus collateral left to the owner of the Cdp when:

• A Cdp with ICR >= MCR is fully redeemed.
• A Cdp with ICR > MCR is fully liquidated in recovery mode.

Any surplus collateral is send to the CollSurplusPool, which can later be claimed by the owner of the Cdp. The only way for an owner to claim his surplus collateral is to call the function claimSurplusCollShares in borrowerOperations, which is the only entry point to the CollSurplusPool.

    /// @notice Claim remaining collateral from a redemption or from a liquidation with ICR > MCR in Recovery Mode
    /// @notice when a Cdp has been fully redeemed from and closed, or liquidated in Recovery Mode with a collateralization ratio higher enough (like over MCR)
    /// @notice the borrower is allowed to claim their stETH collateral surplus that remains in the system if any
    function claimSurplusCollShares() external override {
        // send ETH from CollSurplus Pool to owner
        collSurplusPool.claimSurplusCollShares(msg.sender);
    }

The problem here is that LeverageMacroReferences don't have build in feature to claim the surplus collateral from its fully redeemed or liquidated Cdps. In this case the only way for LeverageMacroReference to claim the surplus collateral is to make an arbitrary call to the function claimSurplusCollShares in borrowerOperations. However this isn't possible as the LeverageMacroReference ensures there aren't any arbitrary calls made to the system contracts.

As we can see In _doSwap the first thing the function does, is to call the internal function _ensureNotSystem which ensures that the arbitraty call isn't made to any of the system contracts. Duo to that the function _doSwap will revert when a LeverageMacroReference tries to make an arbitrary call to borrowerOperations.

    /// @dev Prevents doing arbitrary calls to protected targets
    function _ensureNotSystem(address addy) internal {
        /// @audit Check and add more if you think it's better
        require(addy != address(borrowerOperations));
        require(addy != address(sortedCdps));
        require(addy != address(activePool));
        require(addy != address(cdpManager));
        require(addy != address(this)); // If it could call this it could fake the forwarded caller
    }

    function _doSwap(SwapOperation memory swapData) internal {
        // Ensure call is safe
        // Block all system contracts
        _ensureNotSystem(swapData.addressForSwap);

        // Exact approve
        // Approve can be given anywhere because this is a router, and after call we will delete all approvals
        IERC20(swapData.tokenForSwap).safeApprove(
            swapData.addressForApprove,
            swapData.exactApproveAmount
        );

        // Call and perform swap
        // NOTE: Technically approval may be different from target, something to keep in mind
        // Call target are limited
        // But technically you could approve w/e you want here, this is fine because the contract is a router and will not hold user funds
        (bool success, ) = excessivelySafeCall(
            swapData.addressForSwap,
            gasleft(),
            0,
            0,
            swapData.calldataForSwap
        );
        require(success, "Call has failed");

        // Approve back to 0
        // Enforce exact approval
        // Can use max because the tokens are OZ
        // val -> 0 -> 0 -> val means this is safe to repeat since even if full approve is unused, we always go back to 0 after
        IERC20(swapData.tokenForSwap).safeApprove(swapData.addressForApprove, 0);

        // Do the balance checks after the call to the aggregator
        _doSwapChecks(swapData.swapChecks);
    }

Since there is no build in feature to claim the surplus collateral and LeverageMacroReference can't make an arbitrary call to borrowerOperations. Any surplus collateral owned by LeverageMacroReference can't be further claimed back and will be permanently stuck in CollSurplusPool.

Proof of Concept

POC steps in the function testLeverageIssue:
- Step 1: Get the macro's owner and check if the owner is the wallet address.
- Step 2: Get data for the function doOperation.
- Step 3: Approve leverage with the Cdps's collateral + liquidator rewards.
- Step 4: Call the function doOperation twice and open two Cdps.
- Step 5: Check one of the Cdps was opened && the macro address is the Cdp owner.
- Step 6: The macro reference always sweeps back to the wallet, but make sure that happens.
- Step 7: Redeem fully back one of the Cdps to gain a surplus collateral in the surplus pool.
- Step 8: Make sure the Cdp was fully redeemed.
- Step 9: Check that the macro received surplus collateral from the redeeming.
- Step 10: Preparing data for the swap and the arbitrary call.
- Step 11: Do arbitrary call to the function claimSurplusCollShares in borrowerOperations.
- Final: Expect revert as the leverage macro can't make arbitrary calls to the system contracts.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import "forge-std/Test.sol";
import {SimplifiedDiamondLike} from "../contracts/SimplifiedDiamondLike.sol";

import {eBTCBaseInvariants} from "./BaseInvariants.sol";
import {LeverageMacroReference} from "../contracts/LeverageMacroReference.sol";
import {LeverageMacroBase} from "../contracts/LeverageMacroBase.sol";
import {ICdpManagerData} from "../contracts/Interfaces/ICdpManagerData.sol";

contract LeverageMacroReferenceIssue is eBTCBaseInvariants {

    address wallet = address(0xbad455);

    LeverageMacroReference macro_reference;
    LeverageMacroBase macro_base;

    function setUp() public override {
        super.setUp();

        connectCoreContracts();
        connectLQTYContractsToCore();

        // straight creating new macro reference for the test
        macro_reference = new LeverageMacroReference(
            address(borrowerOperations),
            address(activePool),
            address(cdpManager),
            address(eBTCToken),
            address(collateral),
            address(sortedCdps),
            wallet
        );

        dealCollateral(wallet, 100e18);
    }

    function testLeverageIssue() public {

        vm.startPrank(wallet);

        // Step 1: Get the macro's owner and check if the owner is the wallet address
        address macroOwner = macro_reference.owner();
        assert(wallet == macroOwner);

        // Step 2: Get data for the function doOperation
        LeverageMacroBase.SwapOperation[] memory _levSwapsBefore;
        LeverageMacroBase.SwapOperation[] memory _levSwapsAfter;

        uint256 netColl = 11e18;

        uint256 grossColl = netColl + cdpManager.LIQUIDATOR_REWARD();

        uint256 debt = _utils.calculateBorrowAmount(
            netColl,
            priceFeedMock.fetchPrice(),
            COLLATERAL_RATIO
        );

        LeverageMacroBase.OpenCdpOperation memory _opData = LeverageMacroBase.OpenCdpOperation(
            debt,
            bytes32(0),
            bytes32(0),
            grossColl
        );

        bytes memory _opDataEncoded = abi.encode(_opData);

        LeverageMacroBase.LeverageMacroOperation memory operation = LeverageMacroBase
            .LeverageMacroOperation(
                address(collateral),
                (grossColl - 0),
                _levSwapsBefore,
                _levSwapsAfter,
                LeverageMacroBase.OperationType.OpenCdpOperation,
                _opDataEncoded
            );

        LeverageMacroBase.PostCheckParams memory postCheckParams = LeverageMacroBase
            .PostCheckParams({
                expectedDebt: LeverageMacroBase.CheckValueAndType({
                    value: 0,
                    operator: LeverageMacroBase.Operator.skip
                }),
                expectedCollateral: LeverageMacroBase.CheckValueAndType({
                    value: 0,
                    operator: LeverageMacroBase.Operator.skip
                }),
                cdpId: bytes32(0),
                expectedStatus: ICdpManagerData.Status.active
            });

        // Step 3: Approve leverage with the Cdps's collateral + liquidator rewards.
        collateral.approve(address(macro_reference), 22e18 + 4e17);

        // Step 4: Call the function doOperation twice and open two Cdps
        // Need more than one Cdp in the system to fully redeem a Cdp.
        macro_reference.doOperation(
                    LeverageMacroBase.FlashLoanType.noFlashloan,
                    0,
                    operation,
                    LeverageMacroBase.PostOperationCheck.openCdp,
                    postCheckParams
                );

        macro_reference.doOperation(
                    LeverageMacroBase.FlashLoanType.noFlashloan,
                    0,
                    operation,
                    LeverageMacroBase.PostOperationCheck.openCdp,
                    postCheckParams
                );

        // Step 5: Check one of the Cdps was opened && the macro address is the Cdp owner
        bytes32 cdpId = sortedCdps.cdpOfOwnerByIndex(address(macro_reference), 0);
        address cdpOwner = sortedCdps.getOwnerAddress(cdpId);
        assert(address(macro_reference) == cdpOwner);

        // Step 6: The macro reference always sweeps back to the wallet, but make sure that happens
        uint256 balance = eBTCToken.balanceOf(wallet);
        assert(balance == (debt * 2)); // The debt of the 2 Cdps opened

        // Step 7: Redeem fully back one of the Cdps to gain a surplus collateral in the surplus pool
        cdpManager.redeemCollateral(debt, cdpId, bytes32(0), bytes32(0), 0, 0, 1e18);

        // Step 8: Make sure the Cdp was fully redeemed
        bool redeemed = sortedCdps.contains(cdpId); // False means its not in the list == redeemed
        assert(redeemed == false);

        // Step 9: Check that the macro received surplus collateral from the redeeming
        uint256 surplusColl = collSurplusPool.getSurplusCollShares(address(macro_reference));
        assert(surplusColl > 0);

        // Step 10: Preparing data for the swap and the arbitrary call.
        LeverageMacroBase.SwapCheck[] memory _swapChecks = new LeverageMacroBase.SwapCheck[](1);
        _swapChecks[0] = LeverageMacroBase.SwapCheck(address(0), 0); // empty

        LeverageMacroBase.SwapOperation memory swap = LeverageMacroBase.SwapOperation(
            address(0),
            address(0),
            0,
            address(borrowerOperations),
            abi.encodeCall(borrowerOperations.claimSurplusCollShares, ()),
            _swapChecks
        );

        // Step 11: Do arbitrary call to the function claimSurplusCollShares in borrowerOperations
        // Expect revert as the macro ensures there aren't calls to the system contracts
        // Macro isn't able to call borrowerOperations, check the function _ensureNotSystem.
        vm.expectRevert();
        _doSwap(swap);

        vm.stopPrank();
    }

    // Removing everything else expect the _ensureNotSystem and the excessivelySafeCall function
    // (don't need the other parts for the issue showcase)
    function _doSwap(LeverageMacroBase.SwapOperation memory swapData) internal {
        // Ensure call is safe
        // Block all system contracts
        _ensureNotSystem(swapData.addressForSwap);

        // Call and perform swap
        // NOTE: Technically approval may be different from target, something to keep in mind
        // Call target are limited
        // But technically you could approve w/e you want here, this is fine because the contract is a router and will not hold user funds
        (bool success, ) = excessivelySafeCall(
            swapData.addressForSwap,
            gasleft(),
            0,
            0,
            swapData.calldataForSwap
        );
        require(success, "Call has failed");
    }

    function excessivelySafeCall(
        address _target,
        uint256 _gas,
        uint256 _value,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                _value, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
            // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
            // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
            // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)
        }
        return (_success, _returnData);
    }

        /// @dev Prevents doing arbitrary calls to protected targets
    function _ensureNotSystem(address addy) internal {
        require(addy != address(borrowerOperations));
        require(addy != address(sortedCdps));
        require(addy != address(activePool));
        require(addy != address(cdpManager));
        require(addy != address(this));
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Allowing LeverageMacroReference to do an arbitrary call to one of the system contracts brings bigger risk. So l would say the best recommendation is to build a feature for the LeverageMacroReference to do a claim surplus operation in order to claim its surplus collateral from CollSurplusPool:

• Adjust the enum to have a claim surplus operations as well

    enum OperationType {
        OpenCdpOperation,
        AdjustCdpOperation,
        CloseCdpOperation,
+       ClaimSurplusOperation 
    }

• Add a new else if statement which is triggered when the operation is ClaimSurplusOperation, in the statement the new internal function _claimSurplusCallback is called.

    function _handleOperation(LeverageMacroOperation memory operation) internal {
        uint256 beforeSwapsLength = operation.swapsBefore.length;
        if (beforeSwapsLength > 0) {
            _doSwaps(operation.swapsBefore);
        }

        // Based on the type we do stuff
        if (operation.operationType == OperationType.OpenCdpOperation) {
            _openCdpCallback(operation.OperationData);
        } else if (operation.operationType == OperationType.CloseCdpOperation) {
            _closeCdpCallback(operation.OperationData);
        } else if (operation.operationType == OperationType.AdjustCdpOperation) {
            _adjustCdpCallback(operation.OperationData);
+       } else if (operation.operationType == OperationType.ClaimSurplusOperation {
           _claimSurplusCallback();
        }

        uint256 afterSwapsLength = operation.swapsAfter.length;
        if (afterSwapsLength > 0) {
            _doSwaps(operation.swapsAfter);
        }
    }

• Create the new internal function _claimSurplusCallback, which makes a call to the function claimSurplusCollShares in borrowerOperations and claims the leverage macro's surplus collateral from CollSurplusPool.

+   function _claimSurplusCallback() internal {
+       borrowerOperations.claimSurplusCollShares();
+   }

• After the recommendation LeverageMacroReferences are able to claim the surplus collateral of their fully redeemed or liquidated Cdps and everyone should be happy.

Assessed type

call/delegatecall

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/LeverageMacroBase.sol#L118-L211

Vulnerability details

Impact

On short explanation, the function cdpOfOwnerByIdx loops through the sorted list from the lowest NICR to the biggest NICR in order to find the correct Cdp position by the provided details:

• address of the CDP owner
• index of the CDP position
• a starting node from where the search to start
• maximum limit of number of Cdps to loop through

    function cdpOfOwnerByIdx(
        address owner,
        uint256 index,
        bytes32 startNodeId,
        uint maxNodes
    ) external view override returns (bytes32, bool) {
        return _cdpOfOwnerByIndex(owner, index, startNodeId, maxNodes);
    }

This feature is implemented in LeverageMacroBase, so when new Cdp position is opened, the user is free to compare and check its new Cdp parameters. However in order to do that the macro does the following things:

• First the system prepares for the post check, by looping through the sorted list and counting all the Cdps owned by the macro. Then returns the value of initialCdpIndex, which is the number of owned Cdps by the macro + the next Cdp which is about to be opened.

        /**
         * SETUP FOR POST CALL CHECK
         */
        uint256 initialCdpIndex;
        if (postCheckType == PostOperationCheck.openCdp) {
            // How to get owner
            // sortedCdps.existCdpOwners(_cdpId);
            initialCdpIndex = sortedCdps.cdpCountOf(address(this));
        }

• Second the post check happens, and with the help of the provided initialCdpIndex the system tries to find the correct Cdp position and compare its parameter details with the expected values.

        /**
         * POST CALL CHECK FOR CREATION
         */
        if (postCheckType == PostOperationCheck.openCdp) {
            // How to get owner
            // sortedCdps.existCdpOwners(_cdpId);
            // initialCdpIndex is initialCdpIndex + 1
            bytes32 cdpId = sortedCdps.cdpOfOwnerByIndex(address(this), initialCdpIndex);

            // Check for param details
            ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(cdpId);
            _doCheckValueType(checkParams.expectedDebt, cdpInfo.debt);
            _doCheckValueType(checkParams.expectedCollateral, cdpInfo.coll);
            require(
                cdpInfo.status == checkParams.expectedStatus,
                "!LeverageMacroReference: openCDP status check"
            );
        }

This feature might work when opening the first Cdp, but won't work correctly when the macro already has few opened Cdp positions. The main problem occurs in the post check, when trying to find the correct Cdp position with the help of the function cdpOfOwnerByIndex, the system simply loops through the sorted list and gets the last Cdp position owned by the macro with the biggest NICR. So in order for the post check to work correctly, the system takes a guess that every newly opened Cdp by the macro will have a bigger NICR than the previous opened Cdp positions, which is unrealistic and won't be the case most of the times.

For the example and the provided POC, user opens two new Cdps and wants to check their position details:

1. First the user opens a Cdp with an ICR of 160%, checking the param details here will be successful as its the only opened and owned Cdp by the macro.
2. Second the user opens a Cdp with an ICR of 110%, in the post check the function cdpOfOwnerByIndex will return the last macro's Cdp position in the sorted list with the biggest NICR, which in fact will be the first opened Cdp position with ICR of 160%, as a result checking the param details for the newly opened Cdp with ICR of 110% will simply fail or be incorrect.

Proof of Concept

Step 1: Get the macro's owner and check if the owner is the wallet address.
Step 2: Prepare data for opening the first Cdp with ICR == 160%
Step 3: Approve leverage with the both Cdps collateral + liquidator rewards.
Step 4: Open the first Cdp with ICR == 160% and check param details.
Step 5: Prepare data for the opening of the second Cdp with ICR == 110%.
Step 6: Open the second Cdp with ICR == 110% and expect revert when checking the param details.
Final: Post check reverts as cdpOfOwnerByIndex returns the last macro Cdp with the biggest NICR.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import "forge-std/Test.sol";
import {eBTCBaseInvariants} from "./BaseInvariants.sol";
import {LeverageMacroReference} from "../contracts/LeverageMacroReference.sol";
import {LeverageMacroBase} from "../contracts/LeverageMacroBase.sol";
import {ICdpManagerData} from "../contracts/Interfaces/ICdpManagerData.sol";

contract LeverageMacroCdpIssue is eBTCBaseInvariants {

    address wallet = address(0xbad455);

    LeverageMacroReference macro_reference;
    LeverageMacroBase macro_base;

    function setUp() public override {
        super.setUp();

        connectCoreContracts();
        connectLQTYContractsToCore();

        macro_reference = new LeverageMacroReference(
            address(borrowerOperations),
            address(activePool),
            address(cdpManager),
            address(eBTCToken),
            address(collateral),
            address(sortedCdps),
            wallet
        );

        dealCollateral(wallet, 100e18);
    }

    function testLeverageMacroCdpIssue() public {

        vm.startPrank(wallet);

        // Step 1: Get the macro's owner and check if the owner is the wallet address
        address macroOwner = macro_reference.owner();
        assert(wallet == macroOwner);

        // Step 2: Prepare data for opening the first Cdp with ICR == 160%
        LeverageMacroBase.SwapOperation[] memory _levSwapsBefore;
        LeverageMacroBase.SwapOperation[] memory _levSwapsAfter;

        uint256 netColl = 11e18;
        uint256 grossColl = netColl + cdpManager.LIQUIDATOR_REWARD();

        uint256 debtCR = _utils.calculateBorrowAmount(
            netColl,
            priceFeedMock.fetchPrice(),
            COLLATERAL_RATIO
        );

        LeverageMacroBase.OpenCdpOperation memory _opData = LeverageMacroBase.OpenCdpOperation(
            debtCR,
            bytes32(0),
            bytes32(0),
            grossColl
        );

        bytes memory _opDataEncoded = abi.encode(_opData);

        LeverageMacroBase.LeverageMacroOperation memory operation = LeverageMacroBase
            .LeverageMacroOperation(
                address(collateral),
                grossColl,
                _levSwapsBefore,
                _levSwapsAfter,
                LeverageMacroBase.OperationType.OpenCdpOperation,
                _opDataEncoded
            );

        LeverageMacroBase.PostCheckParams memory postCheckParams = LeverageMacroBase
            .PostCheckParams({
                expectedDebt: LeverageMacroBase.CheckValueAndType({
                    value: debtCR,
                    operator: LeverageMacroBase.Operator.equal
                }),
                expectedCollateral: LeverageMacroBase.CheckValueAndType({
                    value: 11e18,
                    operator: LeverageMacroBase.Operator.equal
                }),
                cdpId: bytes32(0),
                expectedStatus: ICdpManagerData.Status.active
            });

        // Step 3: Approve leverage with the both Cdps collateral + liquidator rewards.
        collateral.approve(address(macro_reference), 22e18 + 4e17);

        // Step 4: Open the first Cdp with ICR == 160% and check param details.
        macro_reference.doOperation(
                    LeverageMacroBase.FlashLoanType.noFlashloan,
                    0,
                    operation,
                    LeverageMacroBase.PostOperationCheck.openCdp,
                    postCheckParams
                );

        // Step 5: Prepare data for the opening of the second Cdp with ICR == 110%.
        uint256 debtMCR = _utils.calculateBorrowAmount(
            netColl,
            priceFeedMock.fetchPrice(),
            MINIMAL_COLLATERAL_RATIO
        );

        LeverageMacroBase.OpenCdpOperation memory _opDataB = LeverageMacroBase.OpenCdpOperation(
            debtMCR,
            bytes32(0),
            bytes32(0),
            grossColl
        );

        bytes memory _opDataEncodedB = abi.encode(_opDataB);

        LeverageMacroBase.LeverageMacroOperation memory operationB = LeverageMacroBase
            .LeverageMacroOperation(
                address(collateral),
                grossColl,
                _levSwapsBefore,
                _levSwapsAfter,
                LeverageMacroBase.OperationType.OpenCdpOperation,
                _opDataEncodedB
            );

        LeverageMacroBase.PostCheckParams memory postCheckParamsB = LeverageMacroBase
            .PostCheckParams({
                expectedDebt: LeverageMacroBase.CheckValueAndType({
                    value: debtMCR,
                    operator: LeverageMacroBase.Operator.equal
                }),
                expectedCollateral: LeverageMacroBase.CheckValueAndType({
                    value: 11e18,
                    operator: LeverageMacroBase.Operator.equal
                }),
                cdpId: bytes32(0),
                expectedStatus: ICdpManagerData.Status.active
            });

        // Step 6: Open the second Cdp with ICR == 110% and expect revert when checking the param details.
        vm.expectRevert();
        macro_reference.doOperation(
                    LeverageMacroBase.FlashLoanType.noFlashloan,
                    0,
                    operationB,
                    LeverageMacroBase.PostOperationCheck.openCdp,
                    postCheckParamsB
                );

        vm.stopPrank();
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

In the current system, there are tons of ways to figure out what will be the correct index for the newly opened Cdp. As an example with the view function getSyncedICR, the user can get the current ICR of his Cdp positions and figure out which will be the correct index to use for the post check when opening the new Cdp.

    function getSyncedICR(bytes32 _cdpId, uint256 _price) public view returns (uint256) {
        uint256 _debt = getSyncedCdpDebt(_cdpId);
        uint256 _collShare = getSyncedCdpCollShares(_cdpId);
        return _calculateCR(_collShare, _debt, _price);
    }

My recommendation would be to remove the setup for post call check and let the user provide his own initial index, which will be used for the post creation call check.

    struct PostCheckParams {
        CheckValueAndType expectedDebt;
        CheckValueAndType expectedCollateral;
        // Used only if cdpStats || isClosed
        bytes32 cdpId;
        // Used only to check status
        ICdpManagerData.Status expectedStatus; // NOTE: THIS IS SUPERFLUOUS
        // Initial index used to find the correct Cdp position
+       uint256 initialCdpIndex;
    }

    function doOperation(
        FlashLoanType flType,
        uint256 borrowAmount,
        LeverageMacroOperation calldata operation,
        PostOperationCheck postCheckType,
        PostCheckParams calldata checkParams
    ) external {
        _assertOwner();

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

        /**
         * POST CALL CHECK FOR CREATION
         */
        if (postCheckType == PostOperationCheck.openCdp) {

+           bytes32 cdpId = sortedCdps.cdpOfOwnerByIndex(address(this), checkParams.initialCdpIndex);

            // Check for param details
            ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(cdpId);
            _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"
            );
        }

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/LiquidationLibrary.sol#L679-L727

Vulnerability details

Impact

Liquidating underwater Cdp in the function batchLiquidateCdps doesn’t redistribute the bad debt accordingly among all other Cdps in the batch list, which results to unfair redistribution of bad debt among all the positions left in the system.

Proof of Concept

The function batchLiquidateCdps is used to liquidate a list of Cdp positions, when the function is called the system liquidates the positions one by one and records the values of:

• the debt that needs to be burned
• the collateral that needs to be send to the liquidator
• the bad debt to redistribute if any
• the collateral reward
• the surplus collateral if any

Basically the system closes all Cdp positions and records the liquidation values which needs to be finalized in order for the liquidation to be successful. The problem here is that this values are finalized at the end of the function when the positions are already closed and all liquidations are already done, as a result if there is bad debt that needs to be redistributed it won't actually affect the Cdps in the batch list, but it will be compensated by all the remaining Cdp positions in the system. This issue leads to unfair redistribution of bad debt when the function batchLiquidateCdps is used to liquidate underwater Cdp.

    function batchLiquidateCdps(bytes32[] memory _cdpArray) external nonReentrantSelfAndBOps {
        require(
            _cdpArray.length != 0,
            "LiquidationLibrary: Calldata address array must not be empty"
        );

        LocalVariables_OuterLiquidationFunction memory vars;
        LiquidationTotals memory totals;

        // taking fee to avoid accounted for the calculation of the TCR
        _syncGlobalAccounting();

        vars.price = priceFeed.fetchPrice();
        (uint256 _TCR, uint256 systemColl, uint256 systemDebt) = _getTCRWithSystemDebtAndCollShares(
            vars.price
        );
        vars.recoveryModeAtStart = _TCR < CCR ? true : false;

        // Perform the appropriate batch liquidation - tally values and obtain their totals.
        if (vars.recoveryModeAtStart) {
            totals = _getTotalFromBatchLiquidate_RecoveryMode(
                vars.price,
                systemColl,
                systemDebt,
                _cdpArray
            );
        } else {
            //  if !vars.recoveryModeAtStart
            totals = _getTotalsFromBatchLiquidate_NormalMode(vars.price, _TCR, _cdpArray);
        }

        require(totals.totalDebtInSequence > 0, "LiquidationLibrary: nothing to liquidate");

        // housekeeping leftover collateral for liquidated CDPs
        if (totals.totalCollSurplus > 0) {
            activePool.transferSystemCollShares(address(collSurplusPool), totals.totalCollSurplus);
        }

        _finalizeLiquidation(
            totals.totalDebtToBurn,
            totals.totalCollToSendToLiquidator,
            totals.totalDebtToRedistribute,
            totals.totalCollReward,
            totals.totalCollSurplus,
            systemColl,
            systemDebt,
            vars.price
        );
    }

Tools Used

Manual review.

Recommended Mitigation Steps

The system should finalize the liquidation values after closing every position, by doing this if there is any bad debt the remaining Cdp positions in the batch list will accordingly take the earned bad debt they need to pay and unfair redistribution won't happen.

Assessed type

Other