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

Lack of checks for token transfer success can lead to unexpected behavior if token transfers fail in `Swan.sol`

safdie

Summary

The Swan smart contract lacks validation checks on the success of ERC20 token transfers (transfer and transferFrom). This omission can lead to unexpected behavior if token transfers fail, resulting in incomplete or erroneous asset sales and royalty distributions.

Vulnerability Details

In the Swan contract, various functions, including transferRoyalties and purchase, perform transfers using the transfer and transferFrom functions of an ERC20 token. However, these functions do not check for success, which is crucial as ERC20 transfers may fail or return false. If these transfers fail silently, funds may not move as expected, causing inconsistent states within the contract and potentially harming users.

Here is an example of the vulnerable code in the transferRoyalties function:

// Function: transferRoyalties
token.transferFrom(asset.seller, address(this), buyerFee);
token.transfer(asset.buyer, buyerFee - driaFee);
token.transfer(owner(), driaFee);

Similarly, in the purchase function:

// Function: purchase
token.transferFrom(listing.buyer, address(this), listing.price);
token.transfer(listing.seller, listing.price);

In these examples, the code assumes that all transfers are successful. Without checking the return value, the contract does not handle possible transfer failures, which may arise if the token has insufficient balance, allowance, or any restrictions in its implementation.

To demonstrate this vulnerability, deploy the Swan contract and simulate a failed token transfer. In this example, I use a custom ERC20 token that always fails on transfer or transferFrom.

Create a Custom ERC20 Token that fails on transfer:

// MaliciousToken.sol
pragma solidity ^0.8.20;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MaliciousToken is ERC20 {
constructor() ERC20("MaliciousToken", "MTK") {
_mint(msg.sender, 1000 * 10 ** 18);
}
​
function transfer(address, uint256) public pure override returns (bool) {
return false;
}
​
function transferFrom(address, address, uint256) public pure override returns (bool) {
return false;
}
}

Hardhat test:

const { expect } = require("chai");
const { ethers } = require("hardhat");
​
describe("Swan Contract Vulnerability PoC", function () {
let Swan, swan, MaliciousToken, token, owner, buyer;
​
beforeEach(async () => {
[owner, buyer] = await ethers.getSigners();
​
// Deploy MaliciousToken
MaliciousToken = await ethers.getContractFactory("MaliciousToken");
token = await MaliciousToken.deploy();
await token.deployed();
​
// Deploy Swan Contract
Swan = await ethers.getContractFactory("Swan");
swan = await Swan.deploy();
await swan.deployed();
​
// Initialize Swan with the malicious token
await swan.initialize(/* params including token address */);
});
​
it("should fail silently on transfer without revert", async () => {
// Owner tries to list an asset
await expect(swan.connect(owner).list("Asset", "AST", "description", 100, buyer.address))
.to.emit(swan, "AssetListed");
​
// Check balance before
const balanceBefore = await token.balanceOf(buyer.address);
​
// Attempt to purchase asset
await swan.connect(buyer).purchase(/* asset address */);
​
// Expect balance to remain unchanged due to failed transfer
const balanceAfter = await token.balanceOf(buyer.address);
expect(balanceAfter).to.equal(balanceBefore, "Balance should not change due to failed transfer.");
});
});

The test indicate that the purchase function does not handle the failed token transfer, resulting in the buyer's balance remaining the same. The asset's status may be erroneously marked as sold despite the failed payment.

Impact

Without transfer success checks, failed transfers can lead to:

  • Inconsistent states, such as assets marked as "sold" without payment being received.

  • Buyers or sellers potentially being denied their expected funds or assets.

  • Incorrect royalty distributions if funds are not transferred to all intended recipients.

These issues can damage trust and lead to potential financial losses for contract users.

Tools Used

Manual review.

Recommendations

Add require statements to check the success of all transfer and transferFrom calls, as shown below:

require(token.transferFrom(asset.seller, address(this), buyerFee), "Transfer failed");
require(token.transfer(asset.buyer, buyerFee - driaFee), "Transfer failed");
require(token.transfer(owner(), driaFee), "Transfer failed");
​
require(token.transferFrom(listing.buyer, address(this), listing.price), "Transfer failed");
require(token.transfer(listing.seller, listing.price), "Transfer failed");

By validating each transfer’s success, the contract will revert if a transfer fails, preventing further execution and maintaining consistency.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[KNOWN] - Low-35 Unsafe use of transfer()/transferFrom() with IERC20

Support

FAQs

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