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

Reentrancy Vulnerability in list Function Allows Over-Listing and Royalty Drain

kunwarsiddarths

Summary

https://github.com/Cyfrin/2024-10-swan-dria/blob/main/contracts/swan/Swan.sol

The list function in the Swan contract is vulnerable to a reentrancy attack, potentially allowing attackers to exceed the maximum asset count per round and repeatedly invoke the transferRoyalties function, draining royalty funds. This vulnerability arises due to the lack of reentrancy protection in the function, coupled with an external call in transferRoyalties, which handles royalty transfers via ERC20 token transfers.

Vulnerability Code

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();
​
// Phase check
if (phase != BuyerAgent.Phase.Sell) {
revert BuyerAgent.InvalidPhase(phase, BuyerAgent.Phase.Sell);
}
​
// Asset count check
if (getCurrentMarketParameters().maxAssetCount == assetsPerBuyerRound[_buyer][round].length) {
revert AssetLimitExceeded(getCurrentMarketParameters().maxAssetCount);
}
​
// Create the asset
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
});
​
assetsPerBuyerRound[_buyer][round].push(asset);
​
// External call vulnerable to reentrancy
transferRoyalties(listings[asset]);
​
emit AssetListed(msg.sender, asset, _price);
}
​

Impact

Exceeding Asset Count: By reentering the list function, attackers can bypass the maxAssetCount check, leading to an overflow of assets listed for a buyer in a particular round.

  • Royalty Drain: The transferRoyalties function transfers royalty fees via ERC20.transfer and ERC20.transferFrom. If reentered, it could result in multiple royalty payments, leading to fund depletion.

Steps To Reproduce

Deploy the Swan Contract with a configured ERC20 token for royalty transfers.

  • Create a malicious contract that calls the list function and reenters the function using the callback from the external transfer in transferRoyalties.

  • Observe the repeated execution of transferRoyalties, allowing royalty funds to be drained and asset count to be exceeded.

Tools Used

Manual Review

Recommendations

Use Reentrancy Guard: Add OpenZeppelin’s ReentrancyGuard to the contract and mark list as nonReentrant. This will prevent reentrant calls to the list function:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract Swan is SwanManager, UUPSUpgradeable, ReentrancyGuard {
function list(...) external nonReentrant {
...
}
}
​

Check-Effects-Interactions Pattern: Refactor list to follow this pattern:

Check: Perform all validations first.
Effects: Update the contract’s internal state (e.g., mappings).
Interactions: Make external calls (like transferRoyalties) after state updates.
Separate Royalty Logic: Consider separating transferRoyalties into a separate function that can be manually invoked or called after listing, avoiding direct invocation within list.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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