c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

If an asset is not withdrawn by its owner, the delegate token holder can continue to flashloan those asset even after the expiry date. This can lead serious issues. For example, well-known NFT projects like AZUKI and BAYC offer benefits to holders, such as the ability to mint new collection NFTs. If an asset is not withdrawn by its owner for some reasons, the delegate token holder can flashloan those assets and use them to mint new NFTs that should belong to the owner.

Proof of Concept

contract MockFlashloan  is IDelegateFlashloan {
    IDelegateToken internal DT;
    constructor(address delegationToken){
        DT = IDelegateToken(delegationToken);
    }

    function onFlashloan(address initiator, Structs.FlashInfo calldata flashInfo) external payable returns (bytes32){
        //Perform actions, such as claiming airdropped tokens.

        //assume the flashloaned asset is an ERC721 token.
        IERC721(flashInfo.tokenContract).approve(address(DT), flashInfo.tokenId);
        return IDelegateFlashloan.onFlashloan.selector;
    }

    function initializeFlashloan(address delegateHolder, IDelegateRegistry.DelegationType DelegateType, address tokenAddress, uint256 tokenId, uint256 amount) external payable {
        DT.flashloan(
            DelegateTokenStructs.FlashInfo(
                address(this),
                delegateHolder,
                DelegateType, 
                tokenAddress,
                tokenId,
                amount,
                bytes("")
        ));
    }
}

    function testFuzzingFlashloadWhenExpiredButHasNotBeenWithdrawn(
        address tokenOwner,
        address dtTo,
        uint256 tokenId,
        bool expiryTypeRelative,
        uint256 time
    ) public {
        vm.assume(tokenOwner != address(0));
        vm.assume(tokenOwner != address(dt));
        vm.assume(tokenOwner != dtTo);
        vm.assume(dtTo != address(0));

        (
            ,
            /* ExpiryType */ uint256 expiry /* expiryValue */,

        ) = prepareValidExpiry(expiryTypeRelative, time);

        mockERC721.mint(tokenOwner, tokenId);
        vm.startPrank(tokenOwner);
        mockERC721.setApprovalForAll(address(dt), true);
        uint256 delegateId = dt.create(
            DelegateTokenStructs.DelegateInfo(
                tokenOwner,
                IDelegateRegistry.DelegationType.ERC721,
                dtTo,
                0,
                address(mockERC721),
                tokenId,
                "",  //assume all rights
                expiry
            ),
            SALT
        );
        vm.stopPrank();

        assertEq(dt.ownerOf(delegateId), dtTo);
        assertEq(principal.ownerOf(delegateId), tokenOwner);
        assertEq(mockERC721.ownerOf(tokenId), address(dt));

        //expired
        vm.warp(expiry + 1 days);

        MockFlashloan nftReceiver = new MockFlashloan(address(dt));
        vm.startPrank(dtTo);
        dt.setApprovalForAll(address(nftReceiver), true);
        vm.stopPrank();  

        nftReceiver.initializeFlashloan(
            dtTo, 
            IDelegateRegistry.DelegationType.ERC721,
            address(mockERC721),
            tokenId,
            0
        );
    }

Tools Used

None

Recommended Mitigation Steps

Do not allow flashloans after the expiry date.

require(StorageHelpers.readExpiry(delegateTokenInfo, delegateTokenId) > block.timestamp, "Expired");

Assessed type

Invalid Validation

c4-judge commented 11 months ago

GalloDaSballo marked the issue as primary issue

GalloDaSballo commented 11 months ago

Has Poc

c4-sponsor commented 11 months ago

0xfoobar (sponsor) disputed

0xfoobar commented 11 months ago

The rescind() function is permissionless so the PT holder or any keeper can rescind post-expiry, no need for extra checks if the PT holder is being lazy for some reason. Good that somebody can claim the utility

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 11 months ago

A gotcha, valid QA imo

RunSoul22 commented 10 months ago

@0xfoobar

The rescind() function is permissionless so the PT holder or any keeper can rescind post-expiry, no need for extra checks.

Although the rescind() function is permissionless, there is no incentive for anyone to do so.

Neither the documentation nor the code demonstrate any plans for keepers to actively monitor delegated tokens and trigger "rescind()" after expiration.

Why bother setting up keepers, which could be costly, when a simple check could solve the problem?

Flashloan Attack on $APE Airdrop In my opinion, this issue has the potential to leak values, so Medium may be appropriate under the C4 criteria.

During the Sprank Contest in CodeHawks, Dacian raised a concern about the potential implications if an auditor identifies a vulnerability in a smart contract and the project defends itself by claiming that an off-chain mechanism would have prevented it. This situation could become problematic for CodeHawks in the future because any project can make such claims about an off-chain component that is not being audited or verifiable.

Dacian further explained that this practice falls into the logical fallacy known as "ad hoc rescue," where one can invent something unverifiable to support their argument or deny an exploit. From his perspective, what matters is what the smart contract allows, rather than relying on unseen, uninspected, and unvalidated off-chain components. https://discord.com/channels/1127263608246636635/1141074733622890556/1144574193816567851 https://discord.com/channels/1127263608246636635/1141074733622890556/1144583303110852671

However, I have great respect for the judge's expertise and will defer to their final decision regarding severity without further dispute.

GalloDaSballo commented 10 months ago

Have asked Judges to reply and share their thoughts on the above, I have written a quite lengthy comment but would like to see other peoples opinion before asserting mine

dmvt commented 10 months ago

I agree with @GalloDaSballo's current ruling of QA. The functionality exists to prevent this situation. Both the team and owner have incentive to call rescind(), so it is not without incentive as stated by the warden. The warden's fix is a valid and potentially useful suggestion, but ultimately a feature request, not a vulnerability, attack vector, or potential loss of funds.