paused ERC721/ERC1155 could cause stopRent to revert, potentially causing issues for the lender.

[c4-bot-5]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L71-L101 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L34 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L42-L50

Vulnerability details

Impact

Many ERC721/ERC1155 tokens, including well-known games such as Axie Infinity, have a pause functionality inside the contract. This pause functionality will cause the stopRent call to always revert and could cause issues, especially for the PAY order type.

Proof of Concept

When stopRent /stopRentBatch is called, it will eventually trigger _reclaimRentedItems and execute reclaimRentalOrder from the safe to send back tokens to lender.

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L353 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L293 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L166-L183

    function _reclaimRentedItems(RentalOrder memory order) internal {
        // Transfer ERC721s from the renter back to lender.
        bool success = ISafe(order.rentalWallet).execTransactionFromModule(
            // Stop policy inherits the reclaimer package.
            address(this),
            // value.
            0,
            // The encoded call to the `reclaimRentalOrder` function.
            abi.encodeWithSelector(this.reclaimRentalOrder.selector, order),
            // Safe must delegate call to the stop policy so that it is the msg.sender.
            Enum.Operation.DelegateCall
        );

        // Assert that the transfer back to the lender was successful.
        if (!success) {
            revert Errors.StopPolicy_ReclaimFailed();
        }
    }

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L71-L101 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L34 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L42-L50

    function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
        // This contract address must be in the context of another address.
        if (address(this) == original) {
            revert Errors.ReclaimerPackage_OnlyDelegateCallAllowed();
        }

        // Only the rental wallet specified in the order can be the address that
        // initates the reclaim. In the context of a delegate call, address(this)
        // will be the safe.
        if (address(this) != rentalOrder.rentalWallet) {
            revert Errors.ReclaimerPackage_OnlyRentalSafeAllowed(
                rentalOrder.rentalWallet
            );
        }

        // Get a count for the number of items.
        uint256 itemCount = rentalOrder.items.length;

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
>>>             _transferERC721(item, rentalOrder.lender);

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
>>>             _transferERC1155(item, rentalOrder.lender);
        }
    }

It can be observed that AxieInfinity has paused functionality : https://etherscan.io/address/0xf5b0a3efb8e8e4c201e2a935f110eaaf3ffecb8d

This is problematic, especially for the PAY order type. Consider a scenario where the lender is not satisfied with how the renter utilizes their PAY order's NFTs. Now, when the lender wants to early stop the rent and calls stopRent, the call will revert, and the earning calculation for the renter will still be growing.

Tools Used

Manual review

Recommended Mitigation Steps

Consider decoupling the NFT claim from stopRent and introducing a timestamp tracker when the lender calls stopRent. Utilize that timestamp value when the rent stop is finalized and calculate the ERC20 reward for the renter.

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

https://github.com/code-423n4/2024-01-renft-findings/issues/130 and https://github.com/code-423n4/2024-01-renft-findings/issues/113 also mentions pause

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Alec1017 (sponsor) acknowledged

[Alec1017]

Non-standard ERC implementations were considered out of scope, but regardless we do plan on adding a whitelist for tokens interacting with the protocol

[0xean]

Not sure this should be left as M, but will leave for now and consider more during PJQA.

Marking as QA is probably more appropriate as the mitigation can potentially lead to as many problems as it solves.

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[stalinMacias]

Marking as QA is probably more appropriate as the mitigation can potentially lead to as many problems as it solves.

@0xean I agree and I believe this one should be a QA at max since the burnable ERC721/1155 bug (much more impactful) is judged as med severity, and here is why:

1. This pausable token will be claimable as soon as they are unpaused by admin, whereas burnable tokens where they are gone forever and will never be reclaimable.
2. The pausable token would only be pausable by an admin, whereas burnable tokens can be burned by anyone as long as he/she owns the NFT they are trying to burn.
3. I believe such pausable tokens are rarer than tokens implementing the burn functionality.
4. And that's the reason you already stated, there really isn't a way to mitigate this just like there isn't a way to mitigate ERC721/1155 tokens that deviate from the standard ERC in addition to deviating from the popular and heavily used extensions (burnable, permit, etc)

[said017]

1. The idea is that a paused token can cause earnings to keep growing. That is why the transfer of NFTs and stopRent should be decoupled. This is a best practice of pull-over-push.

2. It has nothing to do with burn functionality and cannot be compared.

3. I provide AxieInfinity as an example, a widely known NFT game in crypto. It is similar to a blacklistable feature for ERC20 tokens. Not all tokens have such functionality, but if it is widely used, it will become something that needs to be always considered. For instance for blacklistable, USDC implements the functionallity, so it is significantly important to consider such tokens.

4. As I provided and mentioned, decoupling the NFT claim and stopRent is a way to do this.

[jorgect207]

Adding to @stalinMacias This issue is bassicly due a Non-standard ERC implementations of the ERC721/1155, wich is the same problem of the #323, This finding should be duplicate of #323

[said017]

I actually have a different issue that is also a duplicate of #323 (#212). This is distinct, it is not something that should or could be managed by the safe _checkTransaction, it should be managed via pull-over-push design. It is an ERC721/1155 extension behavior that could lead to a different issue.

By the way, I do not necessarily agree with the term "Non-standard ERC". Just because ERC721/ERC1155 has extension functionality doesn't mean it is not ERC721/ERC1155. In fact, OpenZeppelin (OZ) also provides some ERC721/ERC1155 extension functionality, which is considered aligned with the ERC721/ERC1155 standard :

https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC721/extensions https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC1155/extensions

[0xean]

Going to leave as judged. In theory the protocol could mitigate this with a pull mechanism which could be executed after the unpause and allow the counterparty to be unaffected. Currently this amounts to a temporary DOS and therefore seems like M is correct. I think its an edge case, but its plausible as the warden has shown.