flashloan function revert when is called by approved operator

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The flashloan function should allow the delegate token owner or an approved operator to borrow underlying tokens for the duration of a single atomic transaction

When an approved operator calls the function, it is checked to determine if it is a valid operator: StorageHelpers.revertNotOperator(accountOperator, info.delegateHolder); after that token is transfered into receiver address and Helpers.revertOnCallingInvalidFlashloan(info); function is called.

The info parameter is a struct that holds the address of the delegate token owner delegateHolder. When the receiver contract does what it needs with the token, it must return the token to the owner to bypass this check:

TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId);

Unfortunately, if the receiver address returns the tokens to info.delegateHolder or msg.sender, the transaction will revert due to the following check:

IERC721(underlyingContract).ownerOf(underlyingTokenId) != msg.sender

Here msg.sender is the address of the approved operator and the transaction will fail.

Every receiver contract will need to implement additional logic to check if tx.origin is different from info.delegateHolder to determine where to return the tokens for the transaction to be successful. This will add more complexity when integrating the contract with the protocol.

Proof of Concept

Test fail with CallerNotOwnerOrInvalidToken() error

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.21;

import {Test} from "forge-std/Test.sol";
import {FlashReentrancyTester} from "./utils/FlashReentrancy.t.sol";
import {DelegateTokenStructs, BaseLiquidDelegateTest, ComputeAddress} from "test/base/BaseLiquidDelegateTest.t.sol";
import {IDelegateRegistry} from "delegate-registry/src/IDelegateRegistry.sol";
import {IDelegateFlashloan, Structs as IDelegateFlashloanStructs} from "src/interfaces/IDelegateFlashloan.sol";
import {ERC721Holder} from "openzeppelin/token/ERC721/utils/ERC721Holder.sol";
import "forge-std/console.sol";
import {IERC721} from "openzeppelin/token/ERC721/IERC721.sol";

contract FlashLoanReceiver is Test, ERC721Holder {
    function onFlashloan(address, IDelegateFlashloanStructs.FlashInfo calldata info) external payable returns (bytes32) {
        // Return NFT
        IERC721(info.tokenContract).safeTransferFrom(
            address(this), 
            info.delegateHolder, // if this is msg.sender will fail again
            info.tokenId
        );
        return IDelegateFlashloan.onFlashloan.selector;
    }
}
contract FlashloanTest is Test, BaseLiquidDelegateTest, ERC721Holder {
    FlashLoanReceiver flashReceiver;

    function setUp() public {
        flashReceiver = new FlashLoanReceiver();
    }

    // forge test -vvvv --match-test testERC721Flash
    function testERC721Flash() public { 
        uint256 erc721TokenId = mockERC721.mintNext(address(this));

        IERC721(address(mockERC721)).approve(address(dt), erc721TokenId);

        address bob = vm.addr(2);

        uint256 delegateTokenId = dt.create(
            DelegateTokenStructs.DelegateInfo(
                bob,                      // Sends principal token to bob
                IDelegateRegistry.DelegationType.ERC721,
                address(this),           // delegateHolder
                0,
                address(mockERC721),
                erc721TokenId,
                "",                      // Default rights to enable flashloan
                block.timestamp + 1 days // expiry;
            ),
            777
        );

        address alice = vm.addr(1);
        dt.setApprovalForAll(alice, true);

        vm.prank(alice);

        // Alice can take flashLoan
        dt.flashloan{value: 0}(IDelegateFlashloanStructs.FlashInfo(
            address(flashReceiver), // receiver
            address(this), // The holder of the delegation
            IDelegateRegistry.DelegationType.ERC721, 
            address(mockERC721), 
            erc721TokenId, 
            0, 
            ""
        ));

    }
}

Tools Used

Mannual Review

Recommended Mitigation Steps

Before calling of Helpers.revertOnCallingInvalidFlashloan(info); change info.delegateHolder to be equal to msg.sender

            RegistryHelpers.revertERC721FlashUnavailable(delegateRegistry, info);

            IERC721(info.tokenContract).transferFrom(address(this), info.receiver, info.tokenId);

+           info.delegateHolder = msg.sender  
            Helpers.revertOnCallingInvalidFlashloan(info);

            TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId);
            TransferHelpers.pullERC721AfterCheck(info.tokenContract, info.tokenId);

Assessed type

Error

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

0xfoobar (sponsor) confirmed

[0xfoobar]

Need to double-check but looks plausible

[GalloDaSballo]

This will not cause any substantial DOS nor breaks the system in any particular way, since the Operator can: -> Transfer to Self -> Perform the operation

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[gkrastenov]

The operator can transfer tokens to self before taking a flash loan, but he will not meet the requirements for rights. Before taking the loan, this requirement should be met.

 RegistryHelpers.revertERC721FlashUnavailable(delegateRegistry, info);

Unfortunately, will not pass because loadFrom(delegateRegistry, RegistryHashes.erc721Hash(address(this), "", info.delegateHolder, info.tokenId, info.tokenContract)) is equal to 0x0000000000000000000000000000000000000001 instead of address(this)

if ( loadFrom(delegateRegistry, RegistryHashes.erc721Hash(address(this), "", info.delegateHolder, info.tokenId, info.tokenContract)) == address(this)
    | loadFrom(delegateRegistry, RegistryHashes.erc721Hash(address(this), "flashloan", info.delegateHolder, info.tokenId, info.tokenContract)) == address(this)
) return;
  revert Errors.ERC721FlashUnavailable();

PoC:

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.21;

import {Test} from "forge-std/Test.sol";
import {FlashReentrancyTester} from "./utils/FlashReentrancy.t.sol";
import {DelegateTokenStructs, BaseLiquidDelegateTest, ComputeAddress} from "test/base/BaseLiquidDelegateTest.t.sol";
import {IDelegateRegistry} from "delegate-registry/src/IDelegateRegistry.sol";
import {IDelegateFlashloan, Structs as IDelegateFlashloanStructs} from "src/interfaces/IDelegateFlashloan.sol";
import {ERC721Holder} from "openzeppelin/token/ERC721/utils/ERC721Holder.sol";
import "forge-std/console.sol";
import {IERC721} from "openzeppelin/token/ERC721/IERC721.sol";

contract FlashLoanReceiverAndApprovedOperator is Test, ERC721Holder {
    function onFlashloan(address, IDelegateFlashloanStructs.FlashInfo calldata info) external payable returns (bytes32) {
        // Business logic
        // ..............
        // ..............

        // Return NFT
        IERC721(info.tokenContract).safeTransferFrom(
            address(this), 
            msg.sender,
            info.tokenId
        );

        return IDelegateFlashloan.onFlashloan.selector;
    }
}
contract ReentrancyTest is Test, BaseLiquidDelegateTest, ERC721Holder {
    FlashLoanReceiverAndApprovedOperator flashReceiver;

    function setUp() public {
        flashReceiver = new FlashLoanReceiverAndApprovedOperator();
    }

    // forge test -vvvv --match-test testERC721Flash
    function testERC721Flash() public { 
        uint256 erc721TokenId = mockERC721.mintNext(address(this));

        IERC721(address(mockERC721)).approve(address(dt), erc721TokenId);

        address bob = vm.addr(2);

        uint256 delegateTokenId = dt.create(
            DelegateTokenStructs.DelegateInfo(
                bob,                      // Sends principal token to bob
                IDelegateRegistry.DelegationType.ERC721,
                address(this),           // delegateHolder
                0,
                address(mockERC721),
                erc721TokenId,
                "",                      // Default rights to enable flashloan
                block.timestamp + 1 days // expiry
            ),
            777
        );

        address alice = vm.addr(7);
        dt.setApprovalForAll(alice, true);
        assertEq(0, dt.balanceOf(address(alice)));

        vm.prank(address(alice));

        // Alice transfer token to self.
         dt.transferFrom(address(this), alice, delegateTokenId);
         assertEq(1, dt.balanceOf(address(alice)));

        // Alice try to take flashLoan.
        dt.flashloan{value: 0}(IDelegateFlashloanStructs.FlashInfo(
            address(flashReceiver),
            address(this),        // delegateHolder  
            IDelegateRegistry.DelegationType.ERC721, 
            address(mockERC721), 
            erc721TokenId, 
            0, 
            ""
        ));

        // It will fail with:
       // [FAIL. Reason: ERC721FlashUnavailable()]
    }
}

[GalloDaSballo]

This is a partial fix as to how the proper order of operations would be performed

The FlashLoanReceiverAndApprovedOperator

Would transfer the DT token to self Then initiate the flashloan as the owner Then return the token at the end

The below POC works as discussed

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.21;

import {Test} from "forge-std/Test.sol";
import {FlashReentrancyTester} from "./utils/FlashReentrancy.t.sol";
import {DelegateTokenStructs, BaseLiquidDelegateTest, ComputeAddress} from "test/base/BaseLiquidDelegateTest.t.sol";
import {IDelegateRegistry} from "delegate-registry/src/IDelegateRegistry.sol";
import {IDelegateFlashloan, Structs as IDelegateFlashloanStructs} from "src/interfaces/IDelegateFlashloan.sol";
import {ERC721Holder} from "openzeppelin/token/ERC721/utils/ERC721Holder.sol";
import "forge-std/console.sol";
import {IERC721} from "openzeppelin/token/ERC721/IERC721.sol";

contract FlashLoanReceiverAndApprovedOperator is Test, ERC721Holder {
    function onFlashloan(address, IDelegateFlashloanStructs.FlashInfo calldata info) external payable returns (bytes32) {
        // Business logic
        // ..............
        // ..............

        // Return NFT
        // IERC721(info.tokenContract).safeTransferFrom(
        //     address(this), 
        //     msg.sender,
        //     info.tokenId
        // );

        return IDelegateFlashloan.onFlashloan.selector;
    }
}
contract ReentrancyTest is Test, BaseLiquidDelegateTest, ERC721Holder {
    FlashLoanReceiverAndApprovedOperator flashReceiver;

    function setUp() public {
        flashReceiver = new FlashLoanReceiverAndApprovedOperator();
    }

    // forge test -vvvv --match-test testERC721Flash
    function testERC721Flash() public { 
        uint256 erc721TokenId = mockERC721.mintNext(address(this));

        IERC721(address(mockERC721)).approve(address(dt), erc721TokenId);

        address bob = vm.addr(2);

        uint256 delegateTokenId = dt.create(
            DelegateTokenStructs.DelegateInfo(
                bob,                      // Sends principal token to bob
                IDelegateRegistry.DelegationType.ERC721,
                address(this),           // delegateHolder
                0,
                address(mockERC721),
                erc721TokenId,
                "",                      // Default rights to enable flashloan
                block.timestamp + 1 days // expiry
            ),
            777
        );

        // Make flashReceiver the Operator due to how repayment in FL Logic Works
        // Flow is:
        // Operator call to start
        // Operator self-transfers
        // Operator calls FL
        // Operator returns token

        // Due to coding setup, we skip the first half by using pranks

        dt.setApprovalForAll(address(flashReceiver), true);
        assertEq(0, dt.balanceOf(address(flashReceiver)));

        vm.startPrank(address(flashReceiver));

        // NOTE: UNSAFE
        // We give approval to DT to take the token back
        mockERC721.setApprovalForAll(address(dt), true);

        // flashReceiver tranfers token to self.
         dt.transferFrom(address(this), address(flashReceiver), delegateTokenId);
         assertEq(1, dt.balanceOf(address(flashReceiver)));

        // Alice try to take flashLoan.
        dt.flashloan{value: 0}(IDelegateFlashloanStructs.FlashInfo(
            address(flashReceiver),
            address(flashReceiver),        // delegateHolder  
            IDelegateRegistry.DelegationType.ERC721, 
            address(mockERC721), 
            erc721TokenId, 
            0, 
            ""
        ));

        // Contract returns the token here
    }
}