C4 Leverage Macro Fixes

[wtj2021]

• [x] https://github.com/code-423n4/2023-10-badger-findings/issues/323
• [x] https://github.com/code-423n4/2023-10-badger-findings/issues/151
• [x] https://github.com/code-423n4/2023-10-badger-findings/issues/112

[dapp-whisperer]

Review Pt. 1

update ISortedCdps interface to expose toCdpId and nextCdpNonce so that consuming contracts can deterministically find cpdId of Cdp that is being created.

LeverageMacroBase

Is now officially abstract. It was always intended to be inherited and is used this way in practice.

New Operation: ClaimSurplusOperation

We now allow users to claim surplus as an action in the flashloan fallback of a leverage op.

This fixes issues where positions opened via a LeverageMacro impl could have no way to get out collateral surplus out (unless the implementing contract also included that functionality separately)

fixes https://github.com/code-423n4/2023-10-badger-findings/issues/323

How it’s done

we have a new distinct OperationType, ClaimSurplusOperation

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

it corresponds with a new PostOperationCheck type, claimSurplus

This allows for confirming the collateral shares that were gained as part of the operation

enum PostOperationCheck {
        openCdp,
        cdpStats,
        isClosed,
        **claimSurplus**
    }

we piggyback on the PostCheckParams.expectedCollateral parameter to verify the value of the expected output from the claim operation.

During a call

The new flashloan callback is correspondingly simple, no parameters are required. We claim the outstanding surplus that belongs to the contract that has the callback context.

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

For collSurplus post-operation checks, we read the stETH shares in the macro before and after the operation and compare them to ensure our shares increased by at least the expected amount.

we then assume all shares that entered the macro as part of the operation are from this.

The option to sweep all collateral to the caller is retained based on the immutable willSweep flag

[GalloDaSballo]

Wdyt about removing the safety checks for targets That way owner can just call w/e target and w/e signature they need

@wtj2021 @dapp-whisperer

[dapp-whisperer]

EnsureNotSystem

In response to https://github.com/code-423n4/2023-10-badger-findings/issues/112, @GalloDaSballo mentions:

I think we wil remove the safety check and add a way to reset allowances

does this imply you support removal of the _ensureNotSystem check to prevent arbitrary calls to the eBTC contracts via _doSwap?

I was wondering if you could expand further on allowance reset comment @GalloDaSballo?

My Take

The reviewer says the following

This will result in the inability to access the borrowerOperations.permitPositionManagerApproval method for authorization

In fact, I think that the restrictions in _ensureNotSystem can be removed. This is a wallet. Any developer can develop a similar wallet according to his own wishes. The restrictions here have little significance for security.

In creating the zaps, we do in fact get around this by wrapping the LeverageMacroBase in a different contract that can call the positionManager functions.

So this is only really relevant for the LeverageMacroReference impl.

[dapp-whisperer]

Wdyt about removing the safety checks for targets That way owner can just call w/e target and w/e signature they need

@wtj2021 @dapp-whisperer

I'm fine removing it as per previous comment. Was writing mine and didn't see yours :)

Can you remind me of the original security reasoning behind the checks? @GalloDaSballo

[CodingNameKiki]

If l understood correctly in this pull request the main changes in LeverageMacro are:

• LM now interacts with the function toCdpId and with the help of the current block number and next cdp nonce finds the correct Cdp id, which is later used to compare values before and after the open Cdp operations.
• New feature is added and as a result now the LM can claim the surplus collateral earned from its liquidated or fully redeemed Cdps and compare the before and after balance of the stETH to check if the desired amount was claimed.
• Apart from opening a Cdp for itself now the LM can interact with the function openCdpFor and open Cdp specifically one someone else behalf.
• Potential removal of the safety checks, which ensure arbitrary calls aren't made to the ebtc contracts.

Post check of claim surplus can revert or compare wrong values if the LM do arbitrary swaps or calls, which involve receiving stETH or spending some of the balance the LM already has.

On short explanation the function doOperation compare the value of the claimed surplus coll by checking the shares balance of LM before and after performing the claim surplus callback. The concern here is that apart from claiming the surplus coll, the user may want to do also a swap or arbitrary call which may lead to the LM receiving more stETH shares or spending the shares the LM already has. Given this concern, it may be harder to check and compare how much of surplus the LM received.

The question is if this is reasonable concern and should it be fixed???

        uint256 collSharesBefore;
        if (postCheckType == PostOperationCheck.claimSurplus) {
            collSharesBefore = stETH.sharesOf(address(this));
        }

    /// @dev Must be memory since we had to decode it
    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.OpenCdpForOperation) {
            _openCdpForCallback(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);
        }
    }

        // Post check type: claim collateral surplus
        if (postCheckType == PostOperationCheck.claimSurplus) {
            uint256 collSharesClaimed = stETH.sharesOf(address(this)) - collSharesBefore;
            _doCheckValueType(checkParams.expectedCollateral, collSharesClaimed);
        }

Removal of the safety checks which ensure the LM doesn't do any calls to the system contracts.

If the reason for removing the safety checks is that the LM can't do arbitrary call to grant a position approval, shouldn't it be better to just create a new feature which does that? Apart from that there could be some wrong outputs e.g if the person decides to open Cdps by using both the swaps and the OpenCdpOperation at the same time, but l do think that's more like an edge case if someone ever do this.

The good news is that by removing the safety checks, the LM can do arbitrary calls to grant position approvals and also make Cdp liquidations.

[CodingNameKiki]

Ok just to confirm that the issues are fixed:

• The system uses the current block number and cdp nonce and with the help of the function toCdpId finds the correct unique cdp id, which will be later opened. So the LM no longer have the problem to compare the values of the wrong Cdp id.
• The system can now claim surplus, there is not really much to look at here as the new functionality is rather simple LM calls the function claimSurplusCollShares in borrowerOperations to get the surplus. The only concern is comparing values the before and after check of the LM stETH balance of shares, this is described in the above snippet.
• The system can now open cdp on someone else behalf, nothing to look at either as the functions openCdp and openCdpFor are almost identical, except that the LM needs to have position approval in order to do that.

Am l missing something else?

[wtj2021]

@CodingNameKiki Yes, that's correct. toCdpId is used to replace cdpOfOwnerByIndex, which can cause issues for large sortedCdp lists.

[wtj2021]

• removed claimSurplus post validation
• added expectedCdpId to _doOperation (internal) for ZapRouter

[CodingNameKiki]

The Leverage macro can revert in some case without the user being in fault.

Will describe the problem in short explanation, take as an example that a user wants to couple of swaps without wanting to do any of the cdp callbacks e.g opening cdp, adjusting cdp and the other...

    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.OpenCdpForOperation) {
            _openCdpForCallback(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);
        }
    }

However if we look at enum OperationsType, we can see that there is NO option and on zero value the LeverageMacro will suggest that the user wants to do a open cdp operation.

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

So the Leverage macro will always do an external call to find the unique id and always trigger the open cdp callback and try to open Cdp with empty operation data, which will revert and this might result to a significant gas loss to the user, if the user is doing a list of swaps and arbitrary calls.

        bytes32 expectedCdpId;
        if (operation.operationType == OperationType.OpenCdpOperation) {
            expectedCdpId = sortedCdps.toCdpId(
                address(this),
                block.number,
                sortedCdps.nextCdpNonce()
            );

        if (operation.operationType == OperationType.OpenCdpOperation) {
            _openCdpCallback(operation.OperationData);

    function _openCdpCallback(bytes memory data) internal {
        OpenCdpOperation memory flData = abi.decode(data, (OpenCdpOperation));

        /**
         * Open CDP and Emit event
         */
        bytes32 _cdpId = borrowerOperations.openCdp(
            flData.eBTCToMint,
            flData._upperHint,
            flData._lowerHint,
            flData.stETHToDeposit
        );
    }

Unnecessary external call to find the the correct unique cdp id, even tho the user inputed no post check to be made.

Regardless if there is no cdp operation or the user simply turns off the post check feature, the LM will still make an external call to the function toCdpId in order to find it.

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

You can see that the post operation check is turned off on zero value.

    enum PostOperationCheck {
        none,
        openCdp,
        cdpStats,
        isClosed
    }

[CodingNameKiki]

Or is the LM designed to always do a cdp operation alongside the arbitrary calls? @wtj2021

Wouldn't it be better to let people do only arbitrary calls if they want, it can be easily fixed by refactoring the enum:

   enum OperationType {
        None,
        OpenCdpOperation,
        OpenCdpForOperation,
        AdjustCdpOperation,
        CloseCdpOperation,
        ClaimSurplusOperation
    }

[GalloDaSballo]

Overall am thinking we could add a performArbitraryCall operation to allow bypassing those checks and allow adding functionality we currently don't have

I would put this as low prio

I'd be interested in a cut version of the macro, that uses the same audited code, but removes some extra pieces as to make it easier to read and to understand

Overall we should look to simplify the code more so than adding a bunch of additional if/elses

[GalloDaSballo]

That said, the claimCollSurplus is fixed, which means that the macro is usable

I wonder if we forgot about other functionality

Generally speaking the Permission Managers are a bad idea to support in the macro as this is a very advanced piece of tech

If we maintain that we don't want to deploy the macro, then I think we should document these issues and keep as is

[CodingNameKiki]

I wonder if we forgot about other functionality

• Apart from redemptions and liquidations which are not supported on the LM, l don't think there is any other functionality missing here.

By removing the safety checks LeverageMacro will be able to access the above functionalities. Also the LM will be able to open multiple Cdps at once, adjust, close etc and will have access to take both flash loans at once, the design right now only allows one operation at a time. Ideally we should decide what is appropriate for the LM to do and instead of removing the checks add some other operation like Alex mentioned.

[wtj2021]

@CodingNameKiki that makes sense, I added a None operation type for doing trades and a check to skip the expectedCdpId if post check type is set to none.

[CodingNameKiki]

Nice, l think everything looks good now.

[dapp-whisperer]

@GalloDaSballo @rayeaster

after discussion is ready to merge down to main. If you of you guys can stamp we can merge.

[CodingNameKiki]

LGTM for merge