Approved operator can't use flashloan with ERC721 because it is not the owner of the token

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/interfaces/IDelegateToken.sol#L99-L103 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L395 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenTransferHelpers.sol#L31-L38

Vulnerability details

Impact

An approved operator can't use the DelegateToken::flashloan() if the escrowed asset is an ERC721.

Proof of Concept

Like it's documented in the IDelegateToken::flashloan(): https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/interfaces/IDelegateToken.sol#L99-L103

"Allows delegate token owner or approved operator to borrow their underlying tokens for the duration of a single atomic transaction."

A potential approved operator on the contract supposed to be able to use the flashloan() function. But if we look a little closer in the funtion we can see for an ERC721 token type, the following check is made:

if (info.tokenType == IDelegateRegistry.DelegationType.ERC721) { ... TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId); ...

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

And the checkERC721BeforePull() function

if (IERC721(underlyingContract).ownerOf(underlyingTokenId) != msg.sender) { revert Errors.CallerNotOwnerOrInvalidToken(); }

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenTransferHelpers.sol#L31-L38

If the msg.sender is not the owner of the escrowed ERC721, the function revert. And because the approved is not the original owner of the NFT, it revert.

It can be problematic if the user who has delegated his token relies on the fact that the approved operator he has chosen can't use the flashloan() function (e.g. the owner of the ERC721 is a smart contract that uses the delegate services and in his architecture, the protocol relies on another specific smart contract to use flashloan()). This can cause a bad user experience.

You can add this test in DelegateToken.t.sol and launch the command with foundry: forge test --match-test testApprovedUserRevertFlashloan -vvvvv PoC :

//Add this contract representing the flashloan receiver
contract DelegateFlashloan {
    error InvalidFlashloan();

    /**
     * @dev Receive a delegate flashloan
     * @param initiator Caller of the flashloan
     * @param flashInfo Info about the flashloan
     * @return selector The function selector for onFlashloan
     */
    function onFlashloan(address initiator, Structs.FlashInfo calldata flashInfo) external payable returns (bytes32) {
        bytes32 returnData = keccak256(abi.encode("returnData"));
        return returnData;
    }
}

function testApprovedUserRevertFlashloan() public {
    //Variables & setup
    address tokenOwner = address(0x12);
    uint256 tokenId = 15;
    uint256 expiry = 836;
    mockERC721.mint(tokenOwner, tokenId);

    //Creation in DelegateToken
    vm.startPrank(tokenOwner);
    mockERC721.setApprovalForAll(address(dt), true);
    uint256 delegateId =
        dt.create(DelegateTokenStructs.DelegateInfo(tokenOwner, IDelegateRegistry.DelegationType.ERC721, tokenOwner, 0, address(mockERC721), tokenId, "", expiry), SALT);
    // dt.getApproved(delegateId);
    // dt.getDelegateInfo(delegateId);

    //Approved 0xaa
    dt.setApprovalForAll(address(0xaa), true);
    vm.stopPrank();

    //Approved 0xaa try to flashloan
    vm.startPrank(address(0xaa));
    DelegateFlashloan dFL = new DelegateFlashloan();
    address receiver = address(0xbb); // The address to receive the loaned assets
    bytes memory arbitraryData = "0x3333";
    vm.expectRevert(DelegateTokenErrors.CallerNotOwnerOrInvalidToken.selector);
    dt.flashloan(DelegateTokenStructs.FlashInfo(address(dFL), tokenOwner, IDelegateRegistry.DelegationType.ERC721, address(mockERC721), tokenId, 0, arbitraryData));
    vm.stopPrank();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Make changes to let the approved operator make the flashloan (e.g. you can add a similar function like checkERC721BeforePull() but in changing the verification to let the approved operator make the flashloan)

Assessed type

Access Control

[c4-judge]

GalloDaSballo marked the issue as duplicate of #279

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[0xfoobar]

Noted, will keep as-is for identical TransferHelper functionality across both create and flashloan functions. Given that ERC20s, ERC721s, and ERC1155s are custodied in the same contract we can't skips checks on token types. The approved operator can either (1) call the flashloan function itself, in which case msg.sender == info.receiver; (2) transfer the asset to msg.sender at the conclusion of its work

[c4-sponsor]

0xfoobar (sponsor) acknowledged