When consideringItem contains ERC777 token, fulfiller can transfer the NFT form Safe Wallet before this rental order is registered in "Storage.sol".

[c4-bot-5]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L733-L776 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L530-L617

Vulnerability details

Impact

When the considerationItems in a rental order contains ERC777 token, it is possible for a fulfiller to transfer the rental NFT from his Safe Wallet before the rental order is registered in "Storage.sol", thus cause the theft of rental NFTs.

Proof of Concept

First, one thing we need to be aware of is "Guard.sol" allows a NFT to be transferred from Safe Wallet if it's not actively in rent.

function _revertSelectorOnActiveRental(
        bytes4 selector,
        address safe,
        address token,
        uint256 tokenId
    ) private view {
        // Check if the selector is allowed.
        if (STORE.isRentedOut(safe, token, tokenId)) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }
    }

The Storage.sol determines if the Safe Wallet has an active rental item for a specific NFT by following the logic below:

function isRentedOut(
        address recipient,
        address token,
        uint256 identifier
    ) external view returns (bool) {
        // calculate the rental ID
        RentalId rentalId = RentalUtils.getItemPointer(recipient, token, identifier);

        // Determine if there is a positive amount
        return rentedAssets[rentalId] != 0;
    }

So, ifrentedAssets[rentalId] is equal to 0, the protocol will think that NFT is not actively in rental and can be transferred out. rentedAssets[rentalId] is updated during STORE.addRentals() within _rentFromZone(), and _rentFromZone() is invoked within validateOrder().

Now here is the attack idea, validateOrder() is called by Seaport after both offerItem and considerationItem are transferred. If the flow can be handed over to fulfiller before Seaport calling validateOrder(), it is possible for the fulfiller to transfer a rental NFT since "Storage.sol" doesn't think that NFT is in active rental.

And ERC777 token satisfies this requirement. It has "beforeTransfer()" hook to hand over the logic flow to fulfiller before transfer the token (considerationItem), so a fulfiller can exploit this to transfer the offerItem (NFT).

Please note in order for this attack to be succeed, the offer item (NFT) must be transferred to the Safe Wallet before considerationItem (ERC777 token) transfer from fulfiller, and this is exactly the transfer order in Seaport! Please refer to this link to confirm that Seaport first transfer offerItem then transfer considerationItem. https://github.com/ProjectOpenSea/seaport-core/blob/2f546b9a0d61a70e1632445cbcb108149a9369ae/src/lib/OrderFulfiller.sol#L148-L335

So the overall attack is like this:

1. Lender (victim) creates a Seaport order, offerItem is a NFT, considerationItem contains ERC777 token.
2. Renter (attacker) fulfill this order, Seaport firstly transfer offerItem (NFT) to the Safe Wallet, then tries to transfer ERC777 token from attacker, ERC777 token will call attacker before the actual transfer happen.
3. Attacker will call Safe Wallet to transfer the NFT to another address, since at this time we haven't reached to ValidateOrder() yet, rentedAssets[rentalId] is still 0, so Guard.sol will think the NFT is not actively rental, so transfer will be succeeded.
4. All other things go as usual, except from the rental NFT has been stolen.

Also please note, even if a lender doesn't require a ERC777 token as consideration item, the attacker can still inserts a ERC777 token through the "tipping" feature in Seaport. I described the "tipping" feature of Seaport in another of my submission in detail, judges can refer that.

So basically, the rental NFT can be stolen due to the existence of ERC777 token in considerationItem (malicious or not).

Tools Used

Manual Review

Recommended Mitigation Steps

Just don't allow the Safe Wallet to be called by owner to transfer the NFT out.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality

[ding99ya]

@0xean Dear judge, thank you for judging this contest! I apologize for the confusing and misleading title which containing ERC777 token here. This issue is about stealing any ERC721/ERC1155 NFT before it is registered, the process I described here is pretty similar to issue #588. In that issue attacker will reenter through onERC1155Receive(), while here ERC777 token makes it possible because its transfer hook allow the token owner to steal the NFT before the rental registration. This is a high issue because as you can see, at the end of this submission, I intentionally mentioned that there is a tipping feature in Seaport, so even if ERC777 token is not considered in design, the attacker can still free to insert any token due to the tipping feature (so that means he can insert any token which implements custom logic in "transfer()"), for the tipping feature you can refer to #139. Once again, thank you for your judging and I hope you can review this issue again.

[stalinMacias]

@0xean

This report seems identical to this one https://github.com/code-423n4/2024-01-renft-findings/issues/462 From #462:

This means that the rentalWallet will be having the rental asset before the consideration token is transferred from the fulfiller. ERC777 tokens allow callbacks to be setup when tokens are transferred from an account. This allows an attacker to use the rented asset before it has configured correctly.

462 discusses the same attack vector as this one, If ERC777 is used as consideration, the exploit will abuse ERC777 callbacks to hijack the NFT prior to the rental getting registered/configured.

The root cause of both this issue and #462 this is the utilization of ERC777 (and therefore it's send/receive callbacks)

Regarding the tipping feature, the warden has already created a separate report discussing how it can be abused https://github.com/code-423n4/2024-01-renft-findings/issues/139

[ding99ya]

@0xean @stalinMacias Thanks Stalin for referring to the issue #462, I disagree this submission is same as that one.

1. That issue only points out a wallet can use a NFT before the registration, it didn't point out the impact of direct theft.
2. In reality the ERC777 using case is low, this submission is high only because the Seaport tipping feature allows the attacker to insert any token and then steal the NFT by reentrancy, no of other issues mentioned this.

So in conclusion, based on C4 rule, that issue 1). Failed to indicate the impact (direct theft) 2). Don't describe the most important reason (tipping feature allows inserting of any token), therefore I think this submission deserves a solo high.

[ding99ya]

That issue focus on the user have the access to a NFT before the hook.onStart() is set up, and nothing about theft or steal, IMHO seems even not related to this issue

[c4-judge]

0xean removed the grade

[c4-judge]

0xean marked the issue as duplicate of #462

[c4-judge]

0xean marked the issue as satisfactory

[ding99ya]

@0xean Thank you for marking this as a high! It looks we have so many comments in this contest so I appreciate your time and efforts! However, I still don't think it should be marked as a duplicate of #462. The tipping feature not only allowing the insert of ERC777 token to steal NFT, but also actually allowing any type of tokens implementing custom "transfer()" logic to steal the funds, this is the reason why this issue is valid as a high and different from other similar stealing NFT issue like #588.

A issue which doesn't describe the theft of NFTs through tipping feature and doesn't indicate the NFT can be stolen shouldn't be marked as a duplicate of this one, this unfortunately applies to 462 because no attack vector to steal NFT is described and no indicated impact.

Thanks for reading my comments!😁

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[0xean]

batching all the ERC777 token issues together. final call.

[ding99ya]

@0xean please don’t just batching them all together, all of these issues mentioned erc777 don’t mean they are the same. Also, it is clear that the NFTs can be stolen any time, so I don’t think medium is appropriate here. Please reconsider it.

[ding99ya]

@0xean or could you forward this to sponsor to see their opinion? This one marks as insufficient so sponsor don’t have a chance to look at it

[ding99ya]

@Alec1017 sorry for bothering you but could you confirm if you think this issue is high and different from the duplicates? This is a serious issue which may result in the loss of funds.

[0xean]

Closing this issue as invalid due to warden's inability to follow C4 rules.

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[c4-judge]

0xean marked the issue as not a duplicate