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

Reentrancy in Swan Purchase Function Leading to Asset and Fund Theft

foxb868

Summary

The purchase() function in Swan.sol is vulnerable to reentrancy attacks through multiple external calls, enabling asset theft and payment manipulation.

Vulnerability Details

https://github.com/Cyfrin/2024-10-swan-dria/blob/c3f6f027ed51dd31f60b224506de2bc847243eb7/contracts/swan/Swan.sol#L289-L300

function purchase(address _asset) external {
AssetListing storage listing = listings[_asset];
​
// asset must be listed to be purchased
if (listing.status != AssetStatus.Listed) {
revert InvalidStatus(listing.status, AssetStatus.Listed);
}
​
// can only the buyer can purchase the asset
if (listing.buyer != msg.sender) {
revert Unauthorized(msg.sender);
}
​
// @bug detected: State change happens before external calls
// This violates CEI (Checks-Effects-Interactions) pattern
listing.status = AssetStatus.Sold;
​
// @bug detected: Multiple external calls without reentrancy protection
// An attacker could reenter here through malicious ERC721 implementation
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);
​
// @bug detected: Additional external calls that could be reentered
// Malicious ERC20 implementation could reenter here
token.transferFrom(listing.buyer, address(this), listing.price);
token.transfer(listing.seller, listing.price);
}

  1. State changes occur before external calls, violating CEI pattern

  2. Multiple external calls to potentially untrusted contracts (ERC20/ERC721)

  3. No reentrancy guard protection

An attacker could:

  • Create malicious token contracts that reenter on transfer

  • Manipulate listing states during reentrant calls

  • Execute multiple purchases of the same asset

  • Drain funds through double spending

  • Manipulate asset ownership through reentrancy during transfers

It is particularly severe since Swan handles both asset transfers and payments, making it an attractive target for reentrancy attacks targeting either token transfers or payments.

Proof of Concept:

  1. Attacker creates malicious ERC20/ERC721 contract that reenters on transfer

  2. Calls purchase() which triggers the malicious reentrant call

  3. State can be manipulated before it's finalized

Impact

  • Double purchases of same asset

  • Token payment manipulation

  • Asset ownership manipulation

  • Fund drainage through repeated calls

Tools Used

Manual Review

Recommendations

+ import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- function purchase(address _asset) external {
+ function purchase(address _asset) external nonReentrant {
AssetListing storage listing = listings[_asset];
// Validate
if (listing.status != AssetStatus.Listed) revert InvalidStatus();
if (listing.buyer != msg.sender) revert Unauthorized();
+ // Get payment first
+ token.transferFrom(listing.buyer, address(this), listing.price);
// Update state
listing.status = AssetStatus.Sold;
// Execute transfers
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);
// Complete payment
+ token.transfer(listing.seller, listing.price);
emit AssetSold(listing.seller, msg.sender, _asset, listing.price);
}

Alternatively:

  • Implement pull-over-push pattern for payments

  • Add timelock mechanism for large transactions

  • Implement strict asset validation checks

  • Add transaction value caps

Updates

Lead Judging Commences

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