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

Incorrect Comparison Operator in maxAssetCount Check Leads to Ambiguous Logic

theirrationalone

Summary

In the Swan::list function, there is ambiguity in the logic used to check if the number of currently listed assets for a specific round has reached the maxAssetCount. The current code compares the asset count with maxAssetCount using the equality operator (==), which may skip enforcement when the asset count exceeds maxAssetCount.

Swan::list function:

function list(string calldata _name, string calldata _symbol, bytes calldata _desc, uint256 _price, address _buyer)
external
{
BuyerAgent buyer = BuyerAgent(_buyer);
(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();
​
// Buyer must be in the sell phase
if (phase != BuyerAgent.Phase.Sell) {
revert BuyerAgent.InvalidPhase(phase, BuyerAgent.Phase.Sell);
}
// Asset count must not exceed `maxAssetCount`
// @info: Incorrect comparison operator `==`, which will skip the check if maxAssetCount < assetsPerBuyerRound count
if (getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length) {
------------------------------------------------------^
revert AssetLimitExceeded(getCurrentMarketParameters().maxAssetCount);
}
​
// All checks pass, create the asset and its listing
address asset = address(swanAssetFactory.deploy(_name, _symbol, _desc, msg.sender));
listings[asset] = AssetListing({
createdAt: block.timestamp,
royaltyFee: buyer.royaltyFee(),
price: _price,
seller: msg.sender,
status: AssetStatus.Listed,
buyer: _buyer,
round: round
});
​
// Add to list of buyer's assets for this round
assetsPerBuyerRound[_buyer][round].push(asset);
​
// Transfer royalties
transferRoyalties(listings[asset]);
​
emit AssetListed(msg.sender, asset, _price);
}

The condition should use <= rather than == to properly restrict the number of listed assets.

Proof of Code

This issue is further demonstrated in the Swan::relist function, where a similar check enforces that the count of assets does not exceed maxAssetCount.

Swan::relist function:

function relist(address _asset, address _buyer, uint256 _price) external {
AssetListing storage asset = listings[_asset];
​
// Only the seller can relist the asset
if (asset.seller != msg.sender) {
revert Unauthorized(msg.sender);
}
​
// Asset must be listed
if (asset.status != AssetStatus.Listed) {
revert InvalidStatus(asset.status, AssetStatus.Listed);
}
​
// Relist only after the asset's listing round has ended
(uint256 oldRound,,) = BuyerAgent(asset.buyer).getRoundPhase();
if (oldRound <= asset.round) {
revert RoundNotFinished(_asset, asset.round);
}
​
// Transition to new buyer
BuyerAgent buyer = BuyerAgent(_buyer);
(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();
​
// Buyer must be in the sell phase
if (phase != BuyerAgent.Phase.Sell) {
revert BuyerAgent.InvalidPhase(phase, BuyerAgent.Phase.Sell);
}
​
// Buyer must not have more assets than `maxAssetCount`
uint256 count = assetsPerBuyerRound[_buyer][round].length;
@> if (count >= getCurrentMarketParameters().maxAssetCount) {
----------------^
revert AssetLimitExceeded(count);
}
​
// Create the new listing
listings[_asset] = AssetListing({
createdAt: block.timestamp,
royaltyFee: buyer.royaltyFee(),
price: _price,
seller: msg.sender,
status: AssetStatus.Listed,
buyer: _buyer,
round: round
});
​
// Add asset to the list for this round
assetsPerBuyerRound[_buyer][round].push(_asset);
​
// Transfer royalties
transferRoyalties(listings[_asset]);
​
emit AssetRelisted(msg.sender, _buyer, _asset, _price);
}

Tools Used

Manual Review

Recommendation

Please update the comparison logic in the Swan::list function as follows:

function list(string calldata _name, string calldata _symbol, bytes calldata _desc, uint256 _price, address _buyer)
external
{
BuyerAgent buyer = BuyerAgent(_buyer);
(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();
​
// Buyer must be in the sell phase
if (phase != BuyerAgent.Phase.Sell) {
revert BuyerAgent.InvalidPhase(phase, BuyerAgent.Phase.Sell);
}
// Asset count must not exceed `maxAssetCount`
// @info: Incorrect comparison operator `==`, which will skip the check if maxAssetCount < assetsPerBuyerRound count
- if (getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length) {
+ if (getCurrentMarketParameters().maxAssetCount <= assetsPerBuyerRound[_buyer][round].length) {
revert AssetLimitExceeded(getCurrentMarketParameters().maxAssetCount);
}
​
// All checks pass, create the asset and its listing
address asset = address(swanAssetFactory.deploy(_name, _symbol, _desc, msg.sender));
listings[asset] = AssetListing({
createdAt: block.timestamp,
royaltyFee: buyer.royaltyFee(),
price: _price,
seller: msg.sender,
status: AssetStatus.Listed,
buyer: _buyer,
round: round
});
​
// Add to list of buyer's assets for this round
assetsPerBuyerRound[_buyer][round].push(asset);
​
// Transfer royalties
transferRoyalties(listings[asset]);
​
emit AssetListed(msg.sender, asset, _price);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[INVALID] List unlimited items

SwanManager::setMarketParameters pushes the new parameters `marketParameters.push(_marketParameters);` After that, when user calls list the protocol computes the round and the phase `(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();` Inside the getRoundPhase function you have this if statement on top: `if (marketParams.length == marketParameterIdx + 1) {`. The setMarketParameters call changed the `marketParams` length, thing which will case the first case to be false and run the else statement. At the end of that statement we see there is a new round. So the second element of this check `(getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length` is zero, because the [round] is fresh.

Appeal created

theirrationalone Submitter
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[INVALID] List unlimited items

SwanManager::setMarketParameters pushes the new parameters `marketParameters.push(_marketParameters);` After that, when user calls list the protocol computes the round and the phase `(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();` Inside the getRoundPhase function you have this if statement on top: `if (marketParams.length == marketParameterIdx + 1) {`. The setMarketParameters call changed the `marketParams` length, thing which will case the first case to be false and run the else statement. At the end of that statement we see there is a new round. So the second element of this check `(getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length` is zero, because the [round] is fresh.

Support

FAQs

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