Root + Impact
The collectUsdcFromSelling function performs a redundant USDC transfer to address(this) (the contract itself), wasting gas without any effect on contract balance or state.
Impact: Unnecessary gas consumption (~34,450 gas per call) and poor code logic that may indicate a misunderstanding of the fee distribution mechanism.
Description
-
Normal behavior: After an NFT is sold, the seller should be able to collect their share of the sale price plus their locked collateral, while the protocol collects fees that accumulate in the contract for later withdrawal by the owner.
-
The issue: The function executes usdc.safeTransfer(address(this), fees); which attempts to transfer fees from the contract... to itself. This operation does nothing to the contract's USDC balance (the fees were already in the contract) but still consumes gas and executes an unnecessary ERC20 transfer event.
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
@> usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
Risk
Likelihood:High
Impact:
-
Gas waste - Each call wastes approximately 34,450 gas (based on PoC measurement). With many sales, this accumulates to significant unnecessary costs for users.
-
Poor code quality - Indicates the developer may have intended to send fees elsewhere (e.g., a treasury address) but mistakenly used address(this). This could hide a more serious logic flaw in fee management.
Proof of Concept
The following Foundry test demonstrates the redundant transfer and measures the gas wasted:
pragma solidity ^0.8.23;
import "forge-std/Test.sol";
import "forge-std/console.sol";
import "../src/NFTDealers.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockUSDC is ERC20 {
uint8 private constant _DECIMALS = 6;
constructor() ERC20("Mock USDC", "USDC") {}
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
function decimals() public view virtual override returns (uint8) {
return _DECIMALS;
}
}
contract CollectUsdcPoC is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
address public deployer = makeAddr("deployer");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
uint256 constant LOCK_AMOUNT = 20 * 1e6;
uint256 constant NFT_PRICE = 2000 * 1e6;
function setUp() public {
usdc = new MockUSDC();
usdc.mint(seller, LOCK_AMOUNT + NFT_PRICE);
usdc.mint(buyer, NFT_PRICE * 2);
vm.startPrank(deployer);
nftDealers = new NFTDealers(
deployer,
address(usdc),
"Test Collection",
"TNFT",
"ipfs://test-image/",
LOCK_AMOUNT
);
nftDealers.whitelistWallet(seller);
nftDealers.revealCollection();
vm.stopPrank();
}
function testRedundantSelfTransfer() public {
vm.startPrank(seller);
usdc.approve(address(nftDealers), LOCK_AMOUNT + NFT_PRICE);
nftDealers.mintNft();
vm.stopPrank();
vm.prank(seller);
nftDealers.list(1, uint32(NFT_PRICE));
vm.startPrank(buyer);
usdc.approve(address(nftDealers), NFT_PRICE);
nftDealers.buy(1);
vm.stopPrank();
vm.startPrank(seller);
uint256 gasBefore = gasleft();
nftDealers.collectUsdcFromSelling(1);
uint256 gasCost = gasBefore - gasleft();
console.log("Gas consumed by collectUsdcFromSelling:", gasCost);
vm.stopPrank();
uint256 expectedFees = nftDealers.totalFeesCollected();
uint256 contractBalance = usdc.balanceOf(address(nftDealers));
console.log("Expected contract balance (fees):", expectedFees);
console.log("Actual contract balance:", contractBalance);
assertEq(contractBalance, expectedFees, "Contract balance should equal fees");
assertTrue(gasCost > 0, "Function should consume gas");
}
}
Test output:
[⠆] Compiling...
[⠔] Compiling 1 files with Solc 0.8.28
[⠒] Solc 0.8.28 finished in 2.09s
Compiler run successful!
Ran 1 test for test/NFTDealers4.t.sol:CollectUsdcPoC
[PASS] testRedundantSelfTransfer() (gas: 327303)
Logs:
Gas consumed by collectUsdcFromSelling: 34450
Expected contract balance (fees): 60000000
Actual contract balance: 60000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 20.44ms (2.75ms CPU time)
Ran 1 test suite in 41.51ms (20.44ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
The contract's USDC balance equals exactly the fees collected - proving the safeTransfer(address(this), fees) had zero effect on the balance while wasting gas.
Recommended Mitigation
Remove the redundant self-transfer line entirely. The fees are already in the contract and are correctly tracked via totalFeesCollected.
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
