Both routers don't support third party delegating operations on CDP

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-zap-router/src/ZapRouterBase.sol#L115

Vulnerability details

Impact

A third party can't do a CDP operation on original borrower's behave.

Proof of Concept

The protocol provides a way for permit handling CDP like ERC20Permit, which requires original borrower's signature to open, adjust or close a CDP. Such feature also presents in zap routers. However, the value passed into permitting function is incorrect, and causing third party address can't operate on CDP on behave of borrowers.

As we can see, both routers implement ZapRouterBase contract, and whenever permitting is required, _permitPositionManagerApproval function will be called:

    function _permitPositionManagerApproval(
        IBorrowerOperations borrowerOperations,
        PositionManagerPermit memory _positionManagerPermit
    ) internal {
        try
            borrowerOperations.permitPositionManagerApproval(
                msg.sender,
                address(this),
                IPositionManagers.PositionManagerApproval.OneTime,
                _positionManagerPermit.deadline,
                _positionManagerPermit.v,
                _positionManagerPermit.r,
                _positionManagerPermit.s
            )
        {} catch {
            /// @notice adding try...catch around to mitigate potential permit front-running
            /// see: https://www.trust-security.xyz/post/permission-denied
        }
    }

Where we notice that the first parameter is set to msg.sender, and in borrowerOperations.permitPositionManagerApproval, the first parameter is borrower, and meaning where the debt will go to:

    function permitPositionManagerApproval(
        address _borrower,
        address _positionManager,
        PositionManagerApproval _approval,
        uint256 _deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external override {
        require(_deadline >= block.timestamp, "BorrowerOperations: Position manager permit expired");

        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                domainSeparator(),
                keccak256(
                    abi.encode(
                        _PERMIT_POSITION_MANAGER_TYPEHASH,
                        _borrower,
                        _positionManager,
                        _approval,
                        _nonces[_borrower]++,
                        _deadline
                    )
                )
            )
        );
        address recoveredAddress = ecrecover(digest, v, r, s);
        require(
            recoveredAddress != address(0) && recoveredAddress == _borrower,
            "BorrowerOperations: Invalid signature"
        );

        _setPositionManagerApproval(_borrower, _positionManager, _approval);
    }

In the above function, we can see that once signature is recovered, it's checked if it's from the original borrower, and in the router's case, it's essentially:

        require(
            recoveredAddress != address(0) && recoveredAddress == ORIGINAL_MSG_SENDER,
            "BorrowerOperations: Invalid signature"
        );

And after the successful execution, the function continues, for example, in EbtcZapRouter._openCdpWithPermit, which is internally called by opening CDP, CDP will be opened for whoever calls the function. This brings an issue for people who want their assets being managed by third party sources, also as mentioned in the leverage router, the permit feature is meant to support apps like instadapp, defi saver and such.

In the scenario where CDP is created, through a delegator, the CDP will be opened for delegator, and collaterals will also be taken from delegator, which is incorrect, as the delegator should operate the CDP with borrower's signature and collaterals should be taken from the borrower but not delegator. The result is, third party delegation will not work.

Example:

1. Alice wants to invest in the protocol, and wants her assets/debts to be managed by Bob, so she opened a CDP on her own, and let Bob handles the rest as she will provide necessary signatures to Bob when needed.
2. Bob sees an opportunity for leveraging, he then notify his intention to Alice, which Alice agrees and provides required signature. Alice also approves the router for future transfers.
3. Bob prepares to adjust the CDP with the leverage router with the signature Alice has provided, but he would fail because the borrower address derived from the CDP ID is not Bob's address.

One may argue that Alice can transfer required amount of tokens to Bob, and let Bob do the rest, but Bob can also take all assets from Alice and keep as his own. The design here is meant to have a third party handle the CDP operations on behaves of borrowers, but this obviously defeats the purpose, and third parties cannot do anything even if they have proper signatures.

Tools Used

Manual review

Recommended Mitigation Steps

Allow callers to specify the address of borrower instead of using msg.sender. And if users have provide a valid signature, check if the borrower is signature signer instead of msg.sender.

Assessed type

Context

[alex-ppg]

The Warden describes a potential feature that the Sponsor should integrate into their protocol, however, they fail to pinpoint any security risk that arises from its absence rendering it a nice-to-have recommendation and thus of QA severity.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c