ETH can be permanently locked during a flashloan

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenLib.sol#L91-L94 https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L389-L409 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/IDelegateRegistry.sol#L12-L19

Vulnerability details

Impact

During a flashloan, a user can also provide an ETH amount, which will be tunneled to the onFlashloan callback. In some scenarios, this might not happen, and the ETH will be permanently locked inside DelegateToken.

Proof of Concept

1. Alice has a delegateToken for an ERC-721. She needs to flashloan this token, and, at the same time, tunnel some ETH to her flashloan receiver:

function revertOnCallingInvalidFlashloan(DelegateTokenStructs.FlashInfo calldata info) internal {
    if (IDelegateFlashloan(info.receiver).onFlashloan{value: msg.value}(msg.sender, info) == IDelegateFlashloan.onFlashloan.selector) return;
    revert IDelegateFlashloan.InvalidFlashloan();
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenLib.sol#L91-L94

2. She calls flashloan with a total msg.value of 5 ETH, but unfortunately, she uses the default value for FlashInfo.tokenType, which is NONE:

enum DelegationType {
    NONE,
    ALL,
    CONTRACT,
    ERC721,
    ERC20,
    ERC1155
}

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/IDelegateRegistry.sol#L12-L19

3. In the actual flashloan call this won't revert as there aren't any safechecks for info.tokenType, so the ETH will be permanently locked inside DelegateToken:

function flashloan(Structs.FlashInfo calldata info) external payable nonReentrant {
    StorageHelpers.revertNotOperator(accountOperator, info.delegateHolder);
    if (info.tokenType == IDelegateRegistry.DelegationType.ERC721) {
        RegistryHelpers.revertERC721FlashUnavailable(delegateRegistry, info);
        IERC721(info.tokenContract).transferFrom(address(this), info.receiver, info.tokenId);
        Helpers.revertOnCallingInvalidFlashloan(info);
        TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId);
        TransferHelpers.pullERC721AfterCheck(info.tokenContract, info.tokenId);
    } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC20) {
        RegistryHelpers.revertERC20FlashAmountUnavailable(delegateRegistry, info);
        SafeERC20.safeTransfer(IERC20(info.tokenContract), info.receiver, info.amount);
        Helpers.revertOnCallingInvalidFlashloan(info);
        TransferHelpers.checkERC20BeforePull(info.amount, info.tokenContract, info.tokenId);
        TransferHelpers.pullERC20AfterCheck(info.tokenContract, info.amount);
    } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
        RegistryHelpers.revertERC1155FlashAmountUnavailable(delegateRegistry, info);
        TransferHelpers.checkERC1155BeforePull(erc1155PullAuthorization, info.amount);
        IERC1155(info.tokenContract).safeTransferFrom(address(this), info.receiver, info.tokenId, info.amount, "");
        Helpers.revertOnCallingInvalidFlashloan(info);
        TransferHelpers.pullERC1155AfterCheck(erc1155PullAuthorization, info.amount, info.tokenContract, info.tokenId);
    }
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L389-L409

Note about severity

Even if this is a user error:

1. I believe this is a valid use-case as the protocol explicitly declared flashloan as payable and the msg.value is tunneled to the receiver
2. The value lost can potentially be very large
3. The probability of this happening is high (as the default info.tokenType is NONE)
4. It's very easy to add some safeguards for this error

I'm flagging this issue as medium risk.

Tools Used

Manual review

Recommended Mitigation Steps

Consider reverting the transaction when there is an invalid info.tokenType:

    function flashloan(Structs.FlashInfo calldata info) external payable nonReentrant {
        StorageHelpers.revertNotOperator(accountOperator, info.delegateHolder);
        if (info.tokenType == IDelegateRegistry.DelegationType.ERC721) {
            RegistryHelpers.revertERC721FlashUnavailable(delegateRegistry, info);
            IERC721(info.tokenContract).transferFrom(address(this), info.receiver, info.tokenId);
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId);
            TransferHelpers.pullERC721AfterCheck(info.tokenContract, info.tokenId);
        } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC20) {
            RegistryHelpers.revertERC20FlashAmountUnavailable(delegateRegistry, info);
            SafeERC20.safeTransfer(IERC20(info.tokenContract), info.receiver, info.amount);
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.checkERC20BeforePull(info.amount, info.tokenContract, info.tokenId);
            TransferHelpers.pullERC20AfterCheck(info.tokenContract, info.amount);
        } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
            RegistryHelpers.revertERC1155FlashAmountUnavailable(delegateRegistry, info);
            TransferHelpers.checkERC1155BeforePull(erc1155PullAuthorization, info.amount);
            IERC1155(info.tokenContract).safeTransferFrom(address(this), info.receiver, info.tokenId, info.amount, "");
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.pullERC1155AfterCheck(erc1155PullAuthorization, info.amount, info.tokenContract, info.tokenId);
        }
+       else {
+           revert Errors.InvalidTokenType(info.tokenType);
+       }
    }

## Assessed type

ETH-Transfer

[0xfoobar]

Incorrect, revertOnCallingInvalidFlashloan() passes along msg.value to the callback

[c4-sponsor]

0xfoobar (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[c4-judge]

GalloDaSballo removed the grade

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

A caller using None would self-rekt

[DadeKuma]

@0xfoobar

Incorrect, revertOnCallingInvalidFlashloan() passes along msg.value to the callback

This is exactly why this is an issue. If the user uses flashloan with some ETH, but info.tokenType is not ERC721/ERC20/ERC1155, this function won't do anything as none of the if statements will be executed.

The transaction will not revert, and those funds will be lost.

@GalloDaSballo IMO, medium severity seems appropriate for the following reason:

"when the value lost is guaranteed to be very large and preventing the mistake has a very low cost, it is reasonable to assign a medium risk rating. The closer the cost/benefit ratio gets to zero, the more likely the issue should be rated QA. Further, if the suggested mitigation would not prevent the error in most cases (like address(0) checks), the issue should likely be QA."

[0xfoobar]

Value lost is not "guaranteed to be very large". This would be an absurd parameterization that nobody uses, and is trivially caught in even the most basic of unit tests on the implementing flashloan. Dev-facing function not user-facing function. Value lost is practically guaranteed to be zero, same likelihood as sending ETH to the zero address

[DadeKuma]

I respectfully disagree with the sponsor, for the following reasons:

This would be an absurd parameterization that nobody uses

I would agree if the ETH were not tunneled back to the flashloan receiver (i.e. payable function for gas optimization), but this is not the case. A user could test it with a small ETH amount, realizing that it works correctly, and then make the mistake described in this issue when using a bigger amount.

Dev-facing function not user-facing function

Anyone can use a flashloan, supposing they have a delegate token?

Value lost is not "guaranteed to be very large" Value lost is practically guaranteed to be zero

I agree that it's not guaranteed to be very large, but it can potentially be: it's definitely not guaranteed to be zero.

same likelihood as sending ETH to the zero address

Disagree. The zero check address is not valuable as there is an infinite number of wrong addresses. This case is different. A simple revert would make this error impossible.

Quoting:

The closer the cost/benefit ratio gets to zero, the more likely the issue should be rated QA

The cost of this fix is extremely low and the benefit is extremely high as it would make this scenario impossible.

I won't dispute further, and leave this as the judge's decision.

[0xfoobar]

Yes, we can make the fix, it's just QA and not Medium. Using a flashloan requires writing a separate receiver contract, it's not something people just call on etherscan. Dev-facing not user-facing.

[GalloDaSballo]

The reason why we have a self-rekt rule is because it's impossible to enumerate all ways for people to cause self-damage if they behave outside of a common-sense degree of rationality

Most developers would read the code, realize the missed enum causes a loss of funds and write a test for it

The finding makes it clear to those reading the report

Accepting this means we should also accept that if the FlashLoan Receiver is improperly implemented, then some airdrop claim would be lost

The contract has to defend itself, and integrators have to defend their own code