reNFT - Giorgio's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L12-L30 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L2

Vulnerability details

Impact

The Guard contract, which is designed to prevent transactions that would move assets out of the Safe Wallet, currently does not provide protection against the burning of assets within the wallet. If an NFT asset is inheriting ERC721Burnable from oz library, the asset can be attributed to address(0).

This is very much severe as this is a popular oz ERC721/ERC11555 extension that is commonly implemented. If the lender suffers from a burnt NFT, the Guard::stopRent() will revert as the lender cannot reclaim his assets anymore.

Proof of Concept

As we can see the guard contract doesn't implement any way of reverting in case the burn() function may be called.

We can even test this hypothesis by implementing a mockERC721Burnable and test if the burn function reverts.

To do so, we need a mock burnable ERC721.

// SPDX-License-Identifier: MIT

// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC721/extensions/ERC721Burnable.sol)

  

pragma solidity ^0.8.20;

  

// import {ERC721Burnable, ERC721} from "@openzeppelin-contracts/token/ERC721/extensions/ERC721Burnable.sol";

import {ERC721Burnable, ERC721} from "@openzeppelin-contracts/token/ERC721/extensions/ERC721Burnable.sol";

import {Ownable} from "@openzeppelin-contracts/access/Ownable.sol";

import {Test} from "forge-std/Test.sol";

  
  

contract MockERC721Burnable is ERC721Burnable, Ownable {

  

constructor() ERC721("Burnable", "BRN") Ownable(msg.sender) {

}

function mint(address to, uint256 id) onlyOwner() public {

_safeMint(to, id);

}

  

}

now we need to modify the AccountCreator so that it will deploy a erc721 burnable as well.

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.20;

  

import {Vm} from "@forge-std/Vm.sol";

import {LibString} from "@solady/utils/LibString.sol";

import {SafeL2} from "@safe-contracts/SafeL2.sol";

  
  

import {MockERC721Burnable} from "@test/mocks/tokens/standard/MockERC721Burnable.sol";

  

import {MockERC20} from "@test/mocks/tokens/standard/MockERC20.sol";

import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol";

import {MockERC1155} from "@test/mocks/tokens/standard/MockERC1155.sol";

import {ProtocolAccount} from "@test/utils/Types.sol";

import {Protocol} from "@test/fixtures/protocol/Protocol.sol";

  

// Creates test accounts to interact with the V3 protocol

contract AccountCreator is Protocol {

// Protocol accounts for testing

ProtocolAccount public alice;

ProtocolAccount public bob;

ProtocolAccount public carol;

ProtocolAccount public dan;

ProtocolAccount public eve;

  

// Mock tokens for testing

MockERC20[] public erc20s;

MockERC721[] public erc721s;

MockERC1155[] public erc1155s;

  

MockERC721Burnable[] public burnables;

  

function setUp() public virtual override {

super.setUp();

  

// deploy 3 erc20 tokens, 3 erc721 tokens, and 3 erc1155 tokens, anddd 3 burnables

_deployTokens(3);

  

// instantiate all wallets and deploy rental safes for each

alice = _fundWalletAndDeployRentalSafe("alice");

bob = _fundWalletAndDeployRentalSafe("bob");

carol = _fundWalletAndDeployRentalSafe("carol");

dan = _fundWalletAndDeployRentalSafe("dan");

eve = _fundWalletAndDeployRentalSafe("eve");

}

  

function _deployTokens(uint256 numTokens) internal {

for (uint256 i; i < numTokens; i++) {

_deployErc20Token();

_deployErc721Token();

_deployErc1155Token();

_deployErc721Burnable();

}

}

  

function _deployErc20Token() internal returns (uint256 i) {

// save the token's index

i = erc20s.length;

  

// deploy the mock token

MockERC20 token = new MockERC20();

  

// push the token to the array of mocks

erc20s.push(token);

  

// set the token label with the index

vm.label(address(token), string.concat("MERC20_", LibString.toString(i)));

}

  

function _deployErc721Token() internal returns (uint256 i) {

// save the token's index

i = erc721s.length;

  

// deploy the mock token

MockERC721 token = new MockERC721();

  

// push the token to the array of mocks

erc721s.push(token);

  

// set the token label with the index

vm.label(address(token), string.concat("MERC721_", LibString.toString(i)));

}

  

function _deployErc721Burnable() internal returns (uint256 i) {

  

i = burnables.length;

  

// deploy the mock token

MockERC721Burnable token = new MockERC721Burnable();

  

// push the token to the array of mocks

burnables.push(token);

  

// set the token label with the index

vm.label(address(token), string.concat("burnable", LibString.toString(i)));

  

}

  

function _deployErc1155Token() internal returns (uint256 i) {

// save the token's index

i = erc1155s.length;

  

// deploy the mock token

MockERC1155 token = new MockERC1155();

  

// push the token to the array of mocks

erc1155s.push(token);

  

// set the token label with the index

vm.label(address(token), string.concat("MERC1155_", LibString.toString(i)));

}

  

function _deployRentalSafe(

address owner,

string memory name

) internal returns (address safe) {

// Deploy a 1/1 rental safe

address[] memory owners = new address[](1);

owners[0] = owner;

safe = factory.deployRentalSafe(owners, 1);

  

// label the contract

vm.label(address(safe), string.concat("RentalSafe_", name));

}

  

function _fundWalletAndDeployRentalSafe(

string memory name

) internal returns (ProtocolAccount memory account) {

// create a wallet with a address, public key, and private key

Vm.Wallet memory wallet = vm.createWallet(name);

  

// deploy a rental safe for the address

address rentalSafe = _deployRentalSafe(wallet.addr, name);

  

// fund the wallet with ether, all erc20s, and approve the conduit for erc20s, erc721s, erc1155s

_allocateTokensAndApprovals(wallet.addr, 10000);

  

// create an account

account = ProtocolAccount({

addr: wallet.addr,

safe: SafeL2(payable(rentalSafe)),

publicKeyX: wallet.publicKeyX,

publicKeyY: wallet.publicKeyY,

privateKey: wallet.privateKey

});

}

  

function _allocateTokensAndApprovals(address to, uint128 amount) internal {

// deal ether to the recipient

vm.deal(to, amount);

  

// mint all erc20 tokens to the recipient

for (uint256 i = 0; i < erc20s.length; ++i) {

erc20s[i].mint(to, amount);

}

  

// set token approvals

_setApprovals(to);

}

  

function _setApprovals(address owner) internal {

// impersonate the owner address

vm.startPrank(owner);

  

// set all approvals for erc20 tokens

for (uint256 i = 0; i < erc20s.length; ++i) {

erc20s[i].approve(address(conduit), type(uint256).max);

}

  

// set all approvals for erc721 tokens

for (uint256 i = 0; i < erc721s.length; ++i) {

erc721s[i].setApprovalForAll(address(conduit), true);

}

  

// set all approvals for erc1155 tokens

for (uint256 i = 0; i < erc1155s.length; ++i) {

erc1155s[i].setApprovalForAll(address(conduit), true);

}

for (uint256 i = 0; i < burnables.length; ++i) {

burnables[i].setApprovalForAll(address(conduit), true);

}

  
  
  

// stop impersonating

vm.stopPrank();

}

}

And here is the passing check transaction test:

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {Enum} from "@safe-contracts/common/Enum.sol";


import {Errors} from "@src/libraries/Errors.sol";
import {
    e721_transfer_from_selector,
    e721_safe_transfer_from_1_selector,
    e721_safe_transfer_from_2_selector,
    e721_approve_selector,
    e1155_safe_transfer_from_selector,
    e1155_safe_batch_transfer_from_selector,
    shared_set_approval_for_all_selector,
    gnosis_safe_set_guard_selector,
    gnosis_safe_enable_module_selector,
    gnosis_safe_disable_module_selector
} from "@src/libraries/RentalConstants.sol";
import {RentalUtils} from "@src/libraries/RentalUtils.sol";
import {RentalAssetUpdate, RentalId} from "@src/libraries/RentalStructs.sol";

import {BaseTestWithoutEngine} from "@test/BaseTest.sol";
import {
    MockHook_Success,
    MockHook_CheckTransactionRevert,
    MockHook_CheckTransactionRequire,
    MockHook_CheckTransactionPanic,
    MockHookErrors
} from "@test/mocks/MockHook.sol";
import {MockTarget} from "@test/mocks/MockTarget.sol";
import {console} from "forge-std/console.sol";

// Tests functionality on the guard related to transaction checking
contract Guard_CheckTransaction_Unit_Test is BaseTestWithoutEngine {
    // Mock target contract that a hook will be placed between
    MockTarget public mockTarget;
    Burnable public burnable;

    function setUp() public override {
        super.setUp();

        // set up a mock target contract
        mockTarget = new MockTarget();
        

    }

    // helper function to check a transaction
    function _checkTransaction(
        address from,
        address to,
        bytes memory transactionCalldata
    ) public {
        vm.prank(from);
        guard.checkTransaction(
            to,
            0 ether,
            transactionCalldata,
            Enum.Operation.Call,
            0,
            0,
            0 ether,
            ZERO_ADDRESS,
            payable(ZERO_ADDRESS),
            bytes(""),
            ZERO_ADDRESS
        );
    }



    function test_with_burnableNFT() public {
        // Create a rentalId array
        RentalAssetUpdate[] memory rentalAssets = new RentalAssetUpdate[](1);
        rentalAssets[0] = RentalAssetUpdate(
            RentalUtils.getItemPointer(address(alice.safe), address(burnables[0]), 0), //recipient , token address, id
            1
        ); // id,  amount

        // Mark the rental as actively rented in storage
        _markRentalsAsActive(rentalAssets);

        


        // Build up the `transferFrom(address from, address to, uint256 tokenId)` calldata
        bytes memory burnCalldata = abi.encodeWithSelector(
            0x42966c68, //this is the burn selector : "burn(uint256)" 0x42966c68
            0
        );

        
        _checkTransaction(
            address(alice.safe),
            address(burnables[0]),
            burnCalldata
        );
        
    }

}

Tools Used

Manual review

Recommended Mitigation Steps

Add the burn signature to the rentalConstants such as :

++ bytes4 constant shared_burn_selector = 0x42966c68;

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L384-L386 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L389-L391

Vulnerability details

Impact

When preparing a struct for EIP-712 hashing, if a struct type includes other struct types, we need to ensure that all the referenced struct types are defined and encoded as part of the overall structure. This is crucial for maintaining a consistent and deterministic hashing process.

However this is not the case for the rentPayloadTypeHash, this hash is missing the embedded Hook struct. failing to do so will lead to inconsistencies in the hashing process.

Proof of Concept

The rentPayloadTypeHash is composed of rentPayloadTypeString , orderMetadataTypeString and orderFulfillmentTypeString. Now if we look at the orderMetadataTypeString, we can see that it references hooks an array of Hook[] structs

bytes memory orderMetadataTypeString = abi.encodePacked(

"OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)");

which is not defined in the rentPayloadTypeHash.

Tools Used

Manual review

EIP712

Recommended Mitigation Steps

In order to have expected behavior of the protocol, it is best to comply with the EIP712 when using it. In order to mitigate this issue add the hook struct to the rentPayloadTypeHash.

//Signer.sol

rentPayloadTypeHash = keccak256(

   abi.encodePacked(

   rentPayloadTypeString,

   orderMetadataTypeString,
   
++ hookTypeString,

   orderFulfillmentTypeString
)

);

Assessed type

Other