Dria

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

Strict Equality : Swan.list(string,string,bytes,uint256,address) (contracts/swan/Swan.sol#157-191) uses a dangerous strict equality: - getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length (contracts/swan/Swan.sol

Summary : The line of code if (getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length) checks if the maxAssetCount is exactly equal to the length of the assetsPerBuyerRound array. This means that if the length of the array is one more than the maxAssetCount, the condition will not be met, and the function will not revert.

Vulnerability Details : This can lead to a situation where the assetsPerBuyerRound array grows beyond the intended limit (maxAssetCount) without triggering the AssetLimitExceeded error. This can cause unexpected behavior, errors, or even security vulnerabilities.

Impact : This can cause unexpected behavior, errors, or even security vulnerabilities.

Proof of concept : Here's a proof of concept for the issue..

Attack Scenario:

  1. An attacker creates a new buyer account and lists 5 assets using the listAsset function.

  2. The attacker then lists a 6th asset using the listAsset function.

Expected Behavior:

The contract should revert with an "AssetLimitExceeded" error when the attacker tries to list the 6th asset, since the maxAssetCount is 5.

The contract does not revert, and the attacker is able to list the 6th asset. This is because the == comparison in the listAsset function only checks if the length of the assetsPerBuyerRound array is exactly equal to the maxAssetCount, and not greater than or equal to.

pragma solidity ^0.8.0;
contract Swan {
uint256 public maxAssetCount = 5;
mapping(address => uint256[]) public assetsPerBuyerRound;
function listAsset(address buyer) public {
// Check if buyer has reached max asset count
if (assetsPerBuyerRound[buyer].length == maxAssetCount) {
revert("AssetLimitExceeded");
}
// Add asset to buyer's round
assetsPerBuyerRound[buyer].push(1);
}
}

Proof of Concept Code :

pragma solidity ^0.8.0;
contract Attack {
Swan public swan;
constructor(address _swan) public {
swan = Swan(_swan);
}
function attack() public {
// Create a new buyer account
address buyer = address(this);
// List 5 assets
for (uint256 i = 0; i < 5; i++) {
swan.listAsset(buyer);
}
// List a 6th asset (should revert, but doesn't)
swan.listAsset(buyer);
}
}

Deployment and Execution:

  1. Deploy the Swan contract with the maxAssetCount set to 5.

  2. Deploy the Attack contract, passing the address of the Swan contract as a constructor argument.

  3. Call the attack function on the Attack contract.

The attack function will successfully list 6 assets for the buyer, despite the maxAssetCount being 5. This demonstrates the issue with the == comparison in the listAsset function.

Tools Used : Slither

Recommendations : By changing the comparison to >= (greater-than-or-equal-to), we ensure that the function reverts if the length of the assetsPerBuyerRound array is greater than or equal to the maxAssetCount. This means that if the length of the array is one more than the maxAssetCount, the condition will be met, and the function will revert with the AssetLimitExceeded error.

Example:

Suppose maxAssetCount is 5, and the assetsPerBuyerRound array has 5 elements. If we add one more element to the array, its length becomes 6.

  • With the original code (==), the condition would not be met, and the function would not revert.

  • With the updated code (>=), the condition would be met, and the function would revert with the AssetLimitExceeded error.

By using >= instead of ==, we ensure that the function behaves correctly and prevents the assetsPerBuyerRound array from growing beyond the intended limit.

Updates

Lead Judging Commences

inallhonesty Lead Judge 7 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.