Summary
Non-atomic operations in Swan's purchase mechanism create asset lockup and payment vulnerabilities through improper state management.
function purchase(address _asset) external {
AssetListing storage listing = listings[_asset];
listing.status = AssetStatus.Sold;
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);
}
Vulnerability Details
There's a potential ownership violation in the two-step transfer process. The contract assumes both transfers will succeed, but if the second transfer fails after the first one succeeds, the asset could become stuck in the Swan contract.
Swan.sol#function purchase: 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];
if (listing.status != AssetStatus.Listed) {
revert InvalidStatus(listing.status, AssetStatus.Listed);
}
if (listing.buyer != msg.sender) {
revert Unauthorized(msg.sender);
}
listing.status = AssetStatus.Sold;
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);
emit AssetSold(listing.seller, msg.sender, _asset, listing.price);
}
First transferFrom moves the asset from seller to Swan
If second transferFrom fails (e.g., due to revert), asset remains in Swan
The listing status is already updated to Sold
No mechanism exists to recover from this state
See this example
1. Initial State:
- Seller lists NFT
- Buyer initiates purchase
2. Attack Sequence:
- purchase() called
- listing.status set to Sold
- First transfer succeeds (seller -> Swan)
- Second transfer fails
- NFT locked in Swan contract
- Status incorrectly marked as Sold
3. Result:
- Asset trapped
- Buyer lost funds
- Seller lost NFT
- Market state corrupted
Impact
Asset can become permanently locked in the Swan contract
Neither buyer nor seller can recover the asset
Breaks NFT ownership invariant
Tools Used
Vs
Recommendations
function purchase(address _asset) external {
AssetListing storage listing = listings[_asset];
+ require(_asset != address(0), "Invalid asset");
+ require(listings[_asset].seller != address(0), "Asset not listed");
+ // Verify payment first
+ token.transferFrom(listing.buyer, address(this), listing.price);
try {
+ // Atomic transfer
+ SwanAsset(_asset).transferFrom(listing.seller, listing.buyer, 1);
+ listing.status = AssetStatus.Sold;
+ token.transfer(listing.seller, listing.price);
} catch {
+ // Rollback payment
+ token.transfer(listing.buyer, listing.price);
revert("Transfer failed");
}
emit AssetSold(listing.seller, msg.sender, _asset, listing.price);
}
