From 938e11b7f6aa8c35800e20d50ceb9fe6358ea7c5 Mon Sep 17 00:00:00 2001 From: Eduardo Melo Date: Wed, 7 Aug 2024 14:56:45 -0300 Subject: [PATCH] feat: adding reentrancy ERC-721 --- contracts/NftRentalMarketplace.sol | 58 ++++++++++++++++++++++-------- test/NftRentalMarketplace.test.ts | 48 +++++++++++++++++++++++++ test/OriumSftMarketplace.test.ts | 29 +++++++++------ 3 files changed, 110 insertions(+), 25 deletions(-) diff --git a/contracts/NftRentalMarketplace.sol b/contracts/NftRentalMarketplace.sol index cf94158..c81059f 100644 --- a/contracts/NftRentalMarketplace.sol +++ b/contracts/NftRentalMarketplace.sol @@ -9,6 +9,7 @@ import { IOriumMarketplaceRoyalties } from './interfaces/IOriumMarketplaceRoyalt import { OwnableUpgradeable } from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; import { Initializable } from '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; import { PausableUpgradeable } from '@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol'; +import { ReentrancyGuardUpgradeable } from '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; import { LibNftRentalMarketplace, RentalOffer, Rental } from './libraries/LibNftRentalMarketplace.sol'; /** @@ -16,7 +17,7 @@ import { LibNftRentalMarketplace, RentalOffer, Rental } from './libraries/LibNft * @dev This contract is used to manage NFTs rentals, powered by ERC-7432 Non-Fungible Token Roles * @author Orium Network Team - developers@orium.network */ -contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgradeable { +contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable { /** ######### Global Variables ########### **/ /// @dev oriumMarketplaceRoyalties stores the collection royalties and fees @@ -92,6 +93,7 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr function initialize(address _owner, address _oriumMarketplaceRoyalties) external initializer { __Pausable_init(); __Ownable_init(); + __ReentrancyGuard_init(); oriumMarketplaceRoyalties = _oriumMarketplaceRoyalties; @@ -146,7 +148,10 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr * @param _offer The rental offer struct. It should be the same as the one used to create the offer. * @param _duration The duration of the rental. */ - function acceptRentalOffer(RentalOffer calldata _offer, uint64 _duration) external whenNotPaused { + function acceptRentalOffer( + RentalOffer calldata _offer, + uint64 _duration + ) external payable whenNotPaused nonReentrant { bytes32 _offerHash = LibNftRentalMarketplace.hashRentalOffer(_offer); uint64 _expirationDate = uint64(block.timestamp + _duration); LibNftRentalMarketplace.validateAcceptRentalOfferParams( @@ -159,15 +164,38 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr _expirationDate ); - LibNftRentalMarketplace.transferFees( - _offer.feeTokenAddress, - owner(), - _offer.lender, - oriumMarketplaceRoyalties, - _offer.tokenAddress, - _offer.feeAmountPerSecond, - _duration - ); + if (_offer.feeTokenAddress == address(0)) { + uint256 totalFeeAmount = _offer.feeAmountPerSecond * _duration; + require(msg.value >= totalFeeAmount, 'NftRentalMarketplace: Incorrect native token amount'); + + uint256 marketplaceFeeAmount = LibNftRentalMarketplace.getAmountFromPercentage( + totalFeeAmount, + IOriumMarketplaceRoyalties(oriumMarketplaceRoyalties).marketplaceFeeOf(_offer.tokenAddress) + ); + IOriumMarketplaceRoyalties.RoyaltyInfo memory royaltyInfo = IOriumMarketplaceRoyalties( + oriumMarketplaceRoyalties + ).royaltyInfoOf(_offer.tokenAddress); + + uint256 royaltyAmount = LibNftRentalMarketplace.getAmountFromPercentage( + totalFeeAmount, + royaltyInfo.royaltyPercentageInWei + ); + uint256 lenderAmount = totalFeeAmount - marketplaceFeeAmount - royaltyAmount; + + payable(owner()).transfer(marketplaceFeeAmount); + payable(royaltyInfo.treasury).transfer(royaltyAmount); + payable(_offer.lender).transfer(lenderAmount); + } else { + LibNftRentalMarketplace.transferFees( + _offer.feeTokenAddress, + owner(), + _offer.lender, + oriumMarketplaceRoyalties, + _offer.tokenAddress, + _offer.feeAmountPerSecond, + _duration + ); + } LibNftRentalMarketplace.grantRoles( oriumMarketplaceRoyalties, @@ -180,8 +208,8 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr ); for (uint256 i = 0; i < _offer.roles.length; i++) { - if(_expirationDate > roleDeadline[_offer.roles[i]][_offer.tokenAddress][_offer.tokenId]) { - roleDeadline[_offer.roles[i]][_offer.tokenAddress][_offer.tokenId] = _expirationDate; + if (_expirationDate > roleDeadline[_offer.roles[i]][_offer.tokenAddress][_offer.tokenId]) { + roleDeadline[_offer.roles[i]][_offer.tokenAddress][_offer.tokenId] = _expirationDate; } } @@ -233,7 +261,7 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr _offer.tokenId, _offer.roles ); - + uint64 _offerDeadline = nonceDeadline[_offer.lender][_offer.nonce]; if (_offerDeadline < uint64(block.timestamp)) { for (uint256 i = 0; i < _offer.roles.length; i++) { @@ -268,6 +296,7 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr * @dev owner will be msg.sender and it must approve the marketplace to revoke the roles. * @param _tokenAddresses The array of tokenAddresses * @param _tokenIds The array of tokenIds + * @param _roleIds The array of roleIds */ function batchRevokeRole( address[] memory _tokenAddresses, @@ -294,7 +323,6 @@ contract NftRentalMarketplace is Initializable, OwnableUpgradeable, PausableUpgr nonceDeadline[msg.sender][_offer.nonce] = uint64(block.timestamp); for (uint256 i = 0; i < _offer.roles.length; i++) { - if (rentals[_offerHash].expirationDate > uint64(block.timestamp)) { roleDeadline[_offer.roles[i]][_offer.tokenAddress][_offer.tokenId] = rentals[_offerHash].expirationDate; } else { diff --git a/test/NftRentalMarketplace.test.ts b/test/NftRentalMarketplace.test.ts index 0677f83..c4446d5 100644 --- a/test/NftRentalMarketplace.test.ts +++ b/test/NftRentalMarketplace.test.ts @@ -358,6 +358,7 @@ describe('NftRentalMarketplace', () => { .to.emit(marketplace, 'RentalStarted') .withArgs(rentalOffer.lender, rentalOffer.nonce, borrower.address, expirationDate) }) + it('Should accept a rental offer more than once', async () => { const rentalExpirationDate1 = Number(await time.latest()) + duration + 1 @@ -451,6 +452,50 @@ describe('NftRentalMarketplace', () => { marketplace.connect(notOperator).acceptRentalOffer(rentalOffer, duration), ).to.be.revertedWith('NftRentalMarketplace: Sender is not allowed to rent this NFT') }) + it('Should revert when accepting a rental offer with insufficient native tokens', async () => { + await time.increase(ONE_DAY) + rentalOffer.nonce = `0x${randomBytes(32).toString('hex')}` + rentalOffer.borrower = borrower.address + rentalOffer.deadline = Number(await time.latest()) + ONE_DAY + rentalOffer.feeTokenAddress = AddressZero + rentalOffer.feeAmountPerSecond = toWei('0.0000001') + await marketplaceRoyalties + .connect(operator) + .setTrustedFeeTokenForToken([rentalOffer.tokenAddress], [AddressZero], [true]) + await marketplace.connect(lender).createRentalOffer(rentalOffer) + + const totalFeeAmount = rentalOffer.feeAmountPerSecond * BigInt(duration) + + const insufficientAmount = totalFeeAmount - toWei('0.00000001') // slightly less than required + await expect( + marketplace.connect(borrower).acceptRentalOffer(rentalOffer, duration, { + value: insufficientAmount.toString(), + }), + ).to.be.revertedWith('NftRentalMarketplace: Incorrect native token amount') + }) + it('Should accept a rental offer with native tokens', async () => { + await time.increase(ONE_DAY) + rentalOffer.nonce = `0x${randomBytes(32).toString('hex')}` + rentalOffer.borrower = borrower.address + rentalOffer.deadline = Number(await time.latest()) + ONE_DAY + rentalOffer.feeTokenAddress = AddressZero + rentalOffer.feeAmountPerSecond = toWei('0.0000001') + await marketplaceRoyalties + .connect(operator) + .setTrustedFeeTokenForToken([rentalOffer.tokenAddress], [AddressZero], [true]) + await marketplace.connect(lender).createRentalOffer(rentalOffer) + + const blockTimestamp = await time.latest() + const expirationDate = blockTimestamp + duration + 1 + + await expect( + marketplace.connect(borrower).acceptRentalOffer(rentalOffer, duration, { + value: totalFeeAmount.toString(), + }), + ) + .to.emit(marketplace, 'RentalStarted') + .withArgs(rentalOffer.lender, rentalOffer.nonce, borrower.address, expirationDate) + }) it('Should NOT accept a rental offer if offer is expired', async () => { // move foward in time to expire the offer const blockTimestamp = await time.latest() @@ -473,6 +518,7 @@ describe('NftRentalMarketplace', () => { marketplace.connect(borrower).acceptRentalOffer(rentalOffer, maxDuration), ).to.be.revertedWith('NftRentalMarketplace: expiration date is greater than offer deadline') }) + describe('Fees', async function () { const feeAmountPerSecond = toWei('1') const feeAmount = feeAmountPerSecond * BigInt(duration) @@ -495,6 +541,7 @@ describe('NftRentalMarketplace', () => { .withArgs(rentalOffer.lender, rentalOffer.nonce, borrower.address, expirationDate) .to.emit(mockERC20, 'Transfer') }) + it('Should accept a rental offer if marketplace fee is 0', async () => { await marketplaceRoyalties .connect(operator) @@ -504,6 +551,7 @@ describe('NftRentalMarketplace', () => { 'RentalStarted', ) }) + it('Should accept a rental offer if royalty fee is 0', async () => { await marketplaceRoyalties .connect(creator) diff --git a/test/OriumSftMarketplace.test.ts b/test/OriumSftMarketplace.test.ts index f0c5f68..9595c4a 100644 --- a/test/OriumSftMarketplace.test.ts +++ b/test/OriumSftMarketplace.test.ts @@ -16,6 +16,7 @@ import { OriumSftMarketplace, SftRolesRegistrySingleRole, SftRolesRegistrySingleRoleLegacy, + ReentrancyAttack, } from '../typechain-types' describe('OriumSftMarketplace', () => { @@ -689,11 +690,6 @@ describe('OriumSftMarketplace', () => { await marketplace.connect(lender).createRentalOffer({ ...rentalOffer, commitmentId: BigInt(0) }) rentalOffer.commitmentId = BigInt(2) - // Check the borrower's balance before the transaction - const borrowerBalanceBefore = await ethers.provider.getBalance(borrower.address) - console.log('BEFORE') - console.log(borrowerBalanceBefore) - const blockTimestamp = (await ethers.provider.getBlock('latest'))?.timestamp const expirationDate = Number(blockTimestamp) + duration + 1 @@ -704,13 +700,26 @@ describe('OriumSftMarketplace', () => { ) .to.emit(marketplace, 'RentalStarted') .withArgs(rentalOffer.lender, rentalOffer.nonce, borrower.address, expirationDate) + }) - // Check the borrower's balance after the transaction - const borrowerBalanceAfter = await ethers.provider.getBalance(borrower.address) - expect(borrowerBalanceAfter).to.be.lt(borrowerBalanceBefore) + it('Should revert when accepting a rental offer with insufficient native tokens', async function () { + await marketplaceRoyalties + .connect(operator) + .setTrustedFeeTokenForToken([rentalOffer.tokenAddress], [AddressZero], [true]) - console.log('AFTER') - console.log(borrowerBalanceAfter) + rentalOffer.feeTokenAddress = AddressZero + rentalOffer.feeAmountPerSecond = toWei('0.0000001') + const totalFeeAmount = rentalOffer.feeAmountPerSecond * BigInt(duration) + rentalOffer.nonce = `0x${randomBytes(32).toString('hex')}` + await marketplace.connect(lender).createRentalOffer({ ...rentalOffer, commitmentId: BigInt(0) }) + rentalOffer.commitmentId = BigInt(2) + + const insufficientAmount = totalFeeAmount - BigInt(toWei('0.00000001')) // slightly less than required + await expect( + marketplace.connect(borrower).acceptRentalOffer(rentalOffer, BigInt(duration), { + value: insufficientAmount.toString(), + }), + ).to.be.revertedWith('OriumSftMarketplace: Insufficient native token amount') }) describe('Fees', async function () {