Dria
Swan
NFTHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Change in Market parameters - `maxAssetCount` abruptly can destroy current listings

heavenz52

Summary

Due to lack of guard rails when changing platformFee, it can cause the contract to end up in an unintentioal state.

Description

In the following example, we start with maxAssetCount equal to 5. Then after we list 2 items, we set the maxAssetCount to be equal to 1. Now this allows us to add 1 more listing while storing a total of 2 listed assets

Proof Of Concept:

describe("hack", () => {
const currRound = 0n;
​
it("should be in sell phase", async function () {
const [round, phase] = await buyerAgent.getRoundPhase();
expect(round).to.be.equal(currRound);
expect(phase).to.be.equal(Phase.Sell);
});
​
it("allows max asset count to be changed in the same round which causes zombie listings", async function () {
​
await swan.connect(dria).setMarketParameters({
withdrawInterval: minutes(30),
sellInterval: minutes(60),
buyInterval: minutes(10),
platformFee: 100n,
maxAssetCount: 5n, // Setting this value to 5
timestamp: 0n,
});
​
await swan.connect(seller).list(NAME, SYMBOL, DESC, PRICE1, await buyerAgent.getAddress());
await swan.connect(seller).list(NAME, SYMBOL, DESC, PRICE2, await buyerAgent.getAddress());
​
await swan.connect(dria).setMarketParameters({
withdrawInterval: minutes(30),
sellInterval: minutes(60),
buyInterval: minutes(10),
platformFee: 100n,
maxAssetCount: 1n, <@ // Setting this value to 1
timestamp: 1n,
});
​
// Listing succeeds
await swan.connect(seller).list(NAME, SYMBOL, DESC, PRICE3, await buyerAgent.getAddress());
​
const listedAssets = await swan.getListedAssets(
await buyerAgent.getAddress(),
0n,
);
expect(listedAssets.length).to.be.equal(0); <@ // This is still 0
​
​
// Adding a new listing will now revert
​
await expect(swan.connect(seller).list(NAME, SYMBOL, DESC, PRICE1, await buyerAgent.getAddress()))
.to.be.revertedWithCustomError(swan, "AssetLimitExceeded")
.withArgs(1n);
​
});
});

Impact

Loss of listings means loss of funds (royalty fee paid by the sellers) when the owner decides to change the maxAssetCount market parameter

Tools used

Manual Analysis

Recommended Mitigations

Checks should be performed in Swan::setMarketParameters(params) function to ensure smooth transition. This can involve time bound operations as well. So in other words, make a proposal to change market parameters. Let the sellers and buyers decide how / when / if they want to participate. They will have time to make that choice then let there be an approve market proposal function which approve the proposal if the conditions for smooth transition are satisfied.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Changes of the `maxAssetCount` and `platformFee` lead to lost funds due to round change

Appeal created

ljj Auditor
8 months ago
robertodf99 Auditor
8 months ago
galturok Auditor
8 months ago
inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Changes of the `maxAssetCount` and `platformFee` lead to lost funds due to round change

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.