Skip to content

[ORE-05M] Potential Loss of Native Funds #512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Parent: v1.1.0 Migration + Simulation
merged 6 commits into from
May 14, 2025
Merged

[ORE-05M] Potential Loss of Native Funds #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ff1e514
Prevent native funds when seller deposit is 0
zajck May 9, 2025
5ef432f
Cover also native self-sale unwrap
zajck May 9, 2025
7a55ec2
Update test/protocol/offerFacet.ts
zajck May 9, 2025
383a143
Update test/protocol/offerFacet.ts
zajck May 9, 2025
01aec3e
Prevent native funds when erc20 offer and sellerDeposit covered from …
zajck May 12, 2025
bb3e4f6
Update test/protocol/offerFacet.ts
zajck May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions contracts/protocol/facets/Offer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,9 @@ contract OfferFacet is Context, OfferErrors, Access, FundsManager, IOfferEvents
*
* Emits VerificationInitiated and ItemPriceObserved events
*
* N.B. If unwrapping using selfSale for an offer that was set up with both verifier fee and seller deposit, the seller deposit must be
* paid in advance using the depositFunds method.
*
* Reverts if:
* - Caller is not the seller's assistant or facilitator
* - If seller deposit is non zero and there are not enough funds to cover it
Expand Down Expand Up @@ -419,7 +422,10 @@ contract OfferFacet is Context, OfferErrors, Access, FundsManager, IOfferEvents
EntityLib.validateSellerAssistantOrFacilitator(sellerId, offer.facilitatorId);
handleBosonSellerDeposit(sellerId, exchangeToken, offer.sellerDeposit);

// WrapType wrapType = _wrapType;
if (_wrapType != FermionTypes.WrapType.SELF_SALE && offer.sellerDeposit == 0 && msg.value != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me why we exclude the case where offer.sellerDeposit != 0.
If an offer.exchangeToken is an ERC20, it doesn't make sense to send native funds on the transaction, so I think it should revert whatever the value of the sellerDeposit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When offer.sellerDeposit != 0, the checks are done in handleBosonSellerDeposit.

At least, they are when availableFunds < _sellerDeposit, but they were missing for the case availableFunds >= _sellerDeposit. I added it now, thanks for asking.

revert FundsErrors.NativeNotAllowed();
}

deriveAndValidatePriceDiscoveryData(tokenId, _priceDiscovery, exchangeToken, _data);

uint256 bosonProtocolFee = getBosonProtocolFee(exchangeToken, _priceDiscovery.price);
Expand Down Expand Up @@ -497,6 +503,7 @@ contract OfferFacet is Context, OfferErrors, Access, FundsManager, IOfferEvents
if (customItemPrice == 0) {
revert InvalidCustomItemPrice();
}

if (exchangeAmount > 0) {
validateIncomingPayment(exchangeToken, exchangeAmount);
transferERC20FromProtocol(exchangeToken, payable(_priceDiscovery.priceDiscoveryContract), exchangeAmount);
Expand Down Expand Up @@ -585,7 +592,7 @@ contract OfferFacet is Context, OfferErrors, Access, FundsManager, IOfferEvents
/**
* Handle Boson seller deposit
*
* If the seller deposit is non zero, the amount must be deposited into Boson so unwrapping can succed.
* If the seller deposit is non zero, the amount must be deposited into Boson so unwrapping can succeed.
* It the seller has some available funds in Fermion, they are used first.
* Otherwise, the seller must provide the missing amount.
*
Expand All @@ -609,6 +616,7 @@ contract OfferFacet is Context, OfferErrors, Access, FundsManager, IOfferEvents
];

if (availableFunds >= _sellerDeposit) {
if (_exchangeToken != address(0) && msg.value > 0) revert FundsErrors.NativeNotAllowed();
decreaseAvailableFunds(_sellerId, _exchangeToken, _sellerDeposit);
} else {
// For offers in native token, the seller deposit cannot be sent at the time of unwrapping.
Expand Down
40 changes: 39 additions & 1 deletion test/protocol/offerFacet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2275,6 +2275,25 @@ describe("Offer", function () {
});
});

it("Native sent to ERC20 offer and seller deposit fully covered", async function () {
await fundsFacet.depositFunds(sellerId, exchangeToken, sellerDeposit);

await expect(
offerFacet.unwrapNFT(tokenId, WrapType.OS_AUCTION, buyerAdvancedOrder, { value: sellerDeposit }),
).to.be.revertedWithCustomError(fermionErrors, "NativeNotAllowed");
});

it("Native sent to ERC20 offer and seller deposit partially covered", async function () {
const remainder = sellerDeposit / 10n;
await fundsFacet.depositFunds(sellerId, exchangeToken, sellerDeposit - remainder);

await mockToken.approve(fermionProtocolAddress, remainder);

await expect(
offerFacet.unwrapNFT(tokenId, WrapType.OS_AUCTION, buyerAdvancedOrder, { value: sellerDeposit }),
).to.be.revertedWithCustomError(fermionErrors, "NativeNotAllowed");
});

it("Price does not cover the verifier fee", async function () {
const minimalPriceNew = calculateMinimalPrice(
verifierFee,
Expand Down Expand Up @@ -3870,6 +3889,14 @@ describe("Offer", function () {
const newBosonProtocolBalance = await mockToken.balanceOf(bosonProtocolAddress);
expect(newBosonProtocolBalance).to.equal(bosonProtocolBalance + fullPrice - openSeaFee);
});

context("Revert reasons", function () {
it("Native funds cannot be sent if seller deposit is 0", async function () {
await expect(
offerFacet.unwrapNFT(tokenId, WrapType.OS_AUCTION, buyerAdvancedOrder, { value: parseEther("1") }),
).to.be.revertedWithCustomError(fermionErrors, "NativeNotAllowed");
});
});
});

context("unwrapToSelf", function () {
Expand Down Expand Up @@ -3939,7 +3966,6 @@ describe("Offer", function () {

bosonProtocolBalance = await ethers.provider.getBalance(bosonProtocolAddress);

// await mockToken.approve(fermionProtocolAddress, minimalPrice);
const tx = await offerFacet.unwrapNFT(tokenId, WrapType.SELF_SALE, selfSaleData, {
value: minimalPrice,
});
Expand Down Expand Up @@ -3976,6 +4002,12 @@ describe("Offer", function () {
.withArgs(minimalPrice, minimalPrice - 1n);
await mockToken.setBurnAmount(0);
});

it("Native funds cannot be sent if seller deposit is 0", async function () {
await expect(
offerFacet.unwrapNFT(tokenId, WrapType.SELF_SALE, selfSaleData, { value: parseEther("1") }),
).to.be.revertedWithCustomError(fermionErrors, "NativeNotAllowed");
});
});
});
});
Expand Down Expand Up @@ -4122,6 +4154,12 @@ describe("Offer", function () {
offerFacet.unwrapNFT(tokenId, WrapType.OS_AUCTION, buyerAdvancedOrder),
).to.be.revertedWithCustomError(fermionErrors, "InvalidUnwrap");
});

it("Native funds cannot be sent if seller deposit is 0", async function () {
await expect(
offerFacet.unwrapNFT(tokenId, WrapType.OS_FIXED_PRICE, encodedPrice, { value: parseEther("1") }),
).to.be.revertedWithCustomError(fermionErrors, "NativeNotAllowed");
});
});
});
});
Expand Down