c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L452 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L291-L310 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L398-L431 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L963-L981 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L553-L594

Vulnerability details

Impact

Borrower authorization and then opencdp cannot be completed in one transaction

Proof of Concept

In the _handleOperation method, the authorization required before executing _openCdpCallback, _closeCdpCallback, and _adjustCdpCallback will be completed in _doSwaps. The specific code is as follows:

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

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

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

Since access to borrowerOperations is restricted in _ensureNotSystem, the code is as follows:

    function _doSwap(SwapOperation memory swapData) internal {
        _ensureNotSystem(swapData.addressForSwap);
        ...
        (bool success, ) = excessivelySafeCall(
            swapData.addressForSwap,
            gasleft(),
            0,
            0,
            swapData.calldataForSwap
        );
        require(success, "Call has failed");

        ...
    }
    function _ensureNotSystem(address addy) internal {
        require(addy != address(borrowerOperations));  // Restrict access to borrowerOperations
        ...
    }

This will result in the inability to access the borrowerOperations.permitPositionManagerApproval method for authorization, and the borrowerOperations.closeCdp and borrowerOperations.adjustCdpWithColl methods called in _closeCdpCallback and _adjustCdpCallback can be delegated to others to call. The code is as follows:

    function closeCdp(bytes32 _cdpId) external override {
        address _borrower = sortedCdps.getOwnerAddress(_cdpId);
        _requireBorrowerOrPositionManagerAndUpdateManagerApproval(_borrower); // Check if there is authorization
        ...

    }

For example, I have a CDP and now I want to entrust SC Wallet to close my CDP. Since I cannot call permitPositionManagerApproval for authorization, I cannot complete this operation in a transaction.

Tools Used

VSCode

Recommended Mitigation Steps

In the _ensureNotSystem method, remove restrictions on borrowerOperations access.

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.