c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Users are incentivized to traded their "airdrop rights". In order to do so, protocol can be used to place a Seaport order. Until the order is matched and filled, the user still could have rights to their NFT. That means, the listed token's price may include potential airdrop to be claimed. But malicious user could monitor mempool to see the match order transaction, and frontrun it, so the new buyer will receive the delegateToken as they should, but without the initial airdrop. A similar issue was present in another contest - here.

Proof of Concept

Essentially, there is no guarantee that upon completing the order, the user will not use their NFT to claim anything like an airdrop, effectively diminishing the potential value right after the buy transaction. If we check the unit test that was already shipped with the protocol with some small added debug prints that show the owner along the way:

function test721OrderFilledByBuyer() public {
        // Approve conduit for token for seller
        vm.prank(seller.addr);
        mockERC721.setApprovalForAll(address(conduit), true);

        // Define 721 order info and mint to seller, and deal expected eth to buyer
        OffererStructs.ERC721Order memory erc721Order = OffererStructs.ERC721Order({
            tokenId: 42,
            info: OffererStructs.Order({
                rights: "",
                tokenContract: address(mockERC721),
                expiryLength: 30 days,
                expiryType: OffererEnums.ExpiryType.relative,
                signerSalt: 9,
                targetToken: OffererEnums.TargetToken.principal
            })
        });
        OfferItem memory offerItem =
            OfferItem({itemType: ItemType.ERC721, token: erc721Order.info.tokenContract, identifierOrCriteria: erc721Order.tokenId, startAmount: 1, endAmount: 1});
        ConsiderationItem memory considerationItem = ConsiderationItem({
            itemType: ItemType.ERC721,
            token: erc721Order.info.tokenContract,
            identifierOrCriteria: erc721Order.tokenId,
            startAmount: 1,
            endAmount: 1,
            recipient: payable(address(createOfferer))
        });
        bytes memory extraData = abi.encode(
            OffererStructs.Context({
                rights: erc721Order.info.rights,
                signerSalt: erc721Order.info.signerSalt,
                expiryLength: erc721Order.info.expiryLength,
                expiryType: erc721Order.info.expiryType,
                targetToken: erc721Order.info.targetToken
            })
        );
        uint256 expectedETH = 0.3 ether;
        vm.deal(buyer.addr, expectedETH);
        mockERC721.mint(seller.addr, erc721Order.tokenId);
        console.log("owner 1: ", mockERC721.ownerOf(erc721Order.tokenId)); //@audit added this
        console.log("buyer: ", buyer.addr); //@audit added this

        // Create order hash and id
        (uint256 createOrderHash, uint256 delegateId) = createOfferer.calculateERC721OrderHashAndId(seller.addr, address(conduit), erc721Order);

        // Build Order
        AdvancedOrder[] memory orders = new AdvancedOrder[](3);
        orders[0] = _createSellerSignedOrder(seller, offerItem, createOrderHash, erc721Order.info.signerSalt, expectedETH);
        orders[1] = _createContractOrder(considerationItem, createOrderHash, extraData);
        orders[2] = _createBuyerFillOrder(buyer, expectedETH);

        // ============== Set Fulfillments ===============
        // Fulfillments tells Seaport how to match the different order components to ensure
        // everyone's conditions are satisfied.
        Fulfillment[] memory fulfillments = new Fulfillment[](3);
        // Seller NFT => WrapReceipt, black line, order 0 offer item 0 matches with order 1 consideration item 0
        fulfillments[0] = _constructFulfillment(0, 0, 1, 0);
        // Wrap Receipt => Seller, red line, order 1 offer item 0 matches with order 0 consideration item 1
        fulfillments[1] = _constructFulfillment(1, 0, 0, 1);
        // Buyer ETH => Seller, blue line, order 2 offer item 0 matches with order 0 consideration item 0
        // offer: (2, 0); consideration: (0, 0); (orderIndex, itemIndex)
        fulfillments[2] = _constructFulfillment(2, 0, 0, 0);
        console.log("owner after create order: ", mockERC721.ownerOf(erc721Order.tokenId)); //@audit added this
        // Match orders
        vm.startPrank(buyer.addr);
        _trackMatchOrderGasBefore();
        seaport.matchAdvancedOrders{value: expectedETH}(orders, new CriteriaResolver[](0), fulfillments, buyer.addr);
        _trackMatchOrderGasAfter();
        vm.stopPrank();
        console.log("nft owner after buy: ", mockERC721.ownerOf(erc721Order.tokenId)); //@audit added this

        // Check //@audit no debug prints needed here, the assertions speak for themselves
        assertTrue(mockERC721.ownerOf(erc721Order.tokenId) == address(dt));
        assertEq(seller.addr.balance, expectedETH);
        assertEq(buyer.addr.balance, 0);
        IDelegateTokenStructs.DelegateInfo memory delegateInfo = dt.getDelegateInfo(delegateId);
        assertEq(dt.ownerOf(delegateId), buyer.addr);
        assertEq(principal.ownerOf(delegateId), seller.addr);
        assertEq(delegateInfo.expiry, block.timestamp + erc721Order.info.expiryLength);

    }

The result is:

Logs:
  owner 1:  0xDFa97bfe5d2b2E8169b194eAA78Fbb793346B174
  buyer:  0x0fF93eDfa7FB7Ad5E962E4C0EdB9207C03a0fe02
  owner after create order:  0xDFa97bfe5d2b2E8169b194eAA78Fbb793346B174
  gas use by matchAdvancedOrder 692514
  nft owner after buy:  0x9a0d27129e92cee1F2Fc38F6a52d777d6B99f074

So there is a time windows where order is placed, and may contain the potential airdrops or other profits for the owner to be claimed, but in the last moment user will claim them, so the buyer will receive less value than expected.

Tools Used

Manual approach

Recommended Mitigation Steps

If order is placed via Delegate protocol, then the NFT could already be put into an escrow at the moment of placing the order, and the user could receive a simple receipt of a deposited token, to be claimed at any time and the order is invalidated then. This way, its potential value is guaranteed to be unchanged, because the NFT is already owned by the protocol.

Of course, there is another way, where just risk is accepted, in order to avoid developing another code. Then the users should be warned that ANY profit coming from holding a bought delegate tokens will be counted AFTER the successful transaction, and any claims that the bought NFT already holds any claimable value should not be trusted.

Assessed type

Other

c4-sponsor commented 1 year ago

0xfoobar marked the issue as disagree with severity

c4-sponsor commented 1 year ago

0xfoobar (sponsor) disputed

0xfoobar commented 1 year ago

This is not a vulnerability, DelegateTokens reflect future utility rights not past ones

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

As discussed by the Sponsor, the buyer is buying the future ability to claim

There is no guarantee of past claims

Such guarantee would have to be offered by a Sale Smart Contract which could have a Observer to determine what the buyer and seller are exchanging

The DT is agnostic to that as simply guarantees future claims

GalloDaSballo commented 1 year ago

QA - L for gotcha