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

State Inconsistency in Purchase Flow Due to CEI Violation

foxb868

Summary

Swan's purchase mechanism where state changes occur before external interactions, potentially leading to asset theft and locked funds.

Looking at the purchase() function in Swan.sol, there are multiple external calls:

  1. Token transfers via transferFrom()

  2. NFT transfers via transferFrom()

The key issue is in the token transfer sequence:

// transfer asset from seller to Swan, and then from Swan to buyer
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);
​
// transfer money
token.transferFrom(listing.buyer, address(this), listing.price);
token.transfer(listing.seller, listing.price);

The bug is that the contract updates the asset status to "Sold" before performing the transfers:

// update asset status to be sold
listing.status = AssetStatus.Sold;

This violates the CEI (Checks-Effects-Interactions) pattern since the state is updated before external calls.

Vulnerability Details

The purchase() function in Swan.sol: https://github.com/Cyfrin/2024-10-swan-dria/blob/c3f6f027ed51dd31f60b224506de2bc847243eb7/contracts/swan/Swan.sol#L276-L302

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: Status updated before transfers - violates CEI pattern
// Risk: Asset could be marked as sold but transfers may fail
listing.status = AssetStatus.Sold;
​
// @bug detected: Multiple external calls after state change
// Risk: If any transfer fails, state remains corrupted
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);
​
// @bug detected: Payment transfers occur last
// Risk: Asset transferred but payment could fail
token.transferFrom(listing.buyer, address(this), listing.price);
token.transfer(listing.seller, listing.price);
​
emit AssetSold(listing.seller, msg.sender, _asset, listing.price);
}

This is dangerous for the protocol because:

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

  2. Multiple dependent transfers create points of failure

  3. Asset ownership could change without payment completing

  4. Failed transfers would leave the contract in an inconsistent state

  5. No way to revert the status change if transfers fail

The sequence of operations should be:

  1. Execute all transfers

  2. Verify success

  3. Update state

  4. Emit event

This vulnerability could lead to asset theft or locked assets if exploited.

  • Any failed transfer will trigger this vulnerability

  • Multiple external calls increase failure probability

Impact

  1. Assets can be marked as sold without payment completing

  2. Seller loses NFT without receiving payment if token transfer fails

  3. Contract state becomes inconsistent with actual ownership

  4. No way to revert the status change

Tools Used

Manual Review

Recommendations

Consider implementing atomic swaps or escrow mechanism to ensure asset and payment transfers occur atomically.

function purchase(address _asset) external {
AssetListing storage listing = listings[_asset];
// Checks
- listing.status = AssetStatus.Sold;
// Interactions
SwanAsset(_asset).transferFrom(listing.seller, address(this), 1);
SwanAsset(_asset).transferFrom(address(this), listing.buyer, 1);
token.transferFrom(listing.buyer, address(this), listing.price);
token.transfer(listing.seller, listing.price);
+ // Effects after successful transfers
+ listing.status = AssetStatus.Sold;
emit AssetSold(listing.seller, msg.sender, _asset, listing.price);
}
Updates

Lead Judging Commences

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