The Guard does NOT prevent burning and pausing of assets while rent is active

[c4-bot-8]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195

Vulnerability details

Impact

While a rental is active, the Guard contract should prevent any token operations such as (transfer, approve, etc.), so that lender assets could not be stolen. Problem is that Guard.sol does not account for burnable & pausable tokens (which are quite common as well). This means lenders could lose such assets if the renter acts maliciously.

Proof of Concept

According to the docs:

The Guard Prevents transfers of ERC721 and ERC1155 tokens while a rental is active, as well as preventing token approvals and enabling of non-whitelisted gnosis safe modules.

The validation of the functions being called happens inside _checkTransaction of Guard.sol:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195

 function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        //@audit burn && pause NOT checked
        if (selector == e721_safe_transfer_from_1_selector) {
            _revertSelectorOnActiveRental(...)
        } else if (selector == e721_safe_transfer_from_2_selector) {
           _revertSelectorOnActiveRental(...)
        } else if (selector == e721_transfer_from_selector) {
           _revertSelectorOnActiveRental(...)
        }

      ...... 
}

Problem is that no validation is made for the following methods:

• burn (ERC721)
• pause (ERC721)
• burnBatch (ERC1155)

This means that a renter could call any of those methods on the assets in his safe and destroy the assets(if calling burn | burnBatch) or pause them and prevent the transfer back to the lender once rental period is over.

I would like to refer to the publicly known issues section on Dishonest ERC721/ERC1155 Implementations, which states:

The Guard contract can only protect against the transfer of tokens that faithfully implement the ERC721/ERC1155 spec. A dishonest implementation that adds an additional function to transfer the token to another wallet cannot be prevented

The mentioned methods are NOT dishonest or some exotic implementation of ERC721/ERC1155. They are part of the official OpenZeppelin contracts for the two standards and are thus wildly used all the time.

• https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721Pausable
• https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721Burnable
• https://docs.openzeppelin.com/contracts/3.x/api/token/erc1155#ERC1155Burnable

Myself as a web3 dev have implemented such tokens 3 times already!

Tools Used

Manual review

Recommended Mitigation Steps

One option is to add the selectors for those methods to the _checkTransaction function.

Even better approach would be to invert the mechanism. Instead of checking for each method separately (the list can get quite long), a whitelist of allowed methods (primarily get methods) could be made. This way any other call would not be allowed, thus making the safe secure from dishonest standart implementations

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #323

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 2 (Med Risk)