Description Expected: After a listing is sold, the seller calls collectUsdcFromSelling(tokenId) exactly once to receive the sale proceeds minus fees and their mint collateral. The claim should be consumed and cannot be collected again. Actual: The seller can call collectUsdcFromSelling(tokenId) repeatedly for the same settled listing. Each call transfers listing.price - fees + collateralForMinting[tokenId] again, so the seller can double-collect when later mints/sales deposit fresh USDC into the contract.
Risk
Impact: High
Likelihood: High
Risk level: High
Root Cause
collectUsdcFromSelling() always pays out listing.price - fees + collateralForMinting[tokenId].
No claim consumption (claimed flag / deletion) is implemented.
collateralForMinting[tokenId] is not zeroed after payout.
onlySeller(tokenId) continues to authorize the original seller.
Proof of Concept Added focused test at NFTDealersTest.t.sol:215.
function testSellerCanDoubleCollectAndStealLaterSaleProceeds() public {
uint256 tokenId = _sellTokenToBuyer(seller, buyer, price);
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
_sellTokenToBuyer(otherSeller, otherBuyer, otherPrice);
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
}
Command:
Recommended Mitigation Consume the claim before transferring tokens (checks-effects-interactions) so a settled listing cannot be collected again.
Option A (recommended): add claimed
function collectUsdcFromSelling(uint256 tokenId) external onlySeller(tokenId) {
Listing storage listing = listings[tokenId];
require(!listing.claimed, "already collected");
listing.claimed = true;
uint256 collateral = collateralForMinting[tokenId];
collateralForMinting[tokenId] = 0;
uint256 fees = _computeFees(listing.price, tokenId);
uint256 payout = listing.price - fees + collateral;
usdc.transfer(listing.seller, payout);
}
Option B: delete listing
## Description
Expected: After a listing is sold, the seller calls `collectUsdcFromSelling(tokenId)` exactly once to receive the sale proceeds minus fees and their mint collateral. The claim should be consumed and cannot be collected again.
Actual: The seller can call `collectUsdcFromSelling(tokenId)` repeatedly for the same settled listing. Each call transfers `listing.price - fees + collateralForMinting[tokenId]` again, allowing double-collecting whenever later mints/sales deposit fresh USDC into the contract.
## Risk
- Impact: High
- Likelihood: High
## Root Cause
- `collectUsdcFromSelling()` pays out `listing.price - fees + collateralForMinting[tokenId]` every call.
- No claim consumption (`claimed` flag/deletion) is implemented.
- `collateralForMinting[tokenId]` is not zeroed after payout.
- `onlySeller(tokenId)` continues to authorize the original seller.
## Proof of Concept
PoC test: `NFTDealersTest.t.sol:215`
```solidity
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
vm.prank(otherSeller);
...
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
```
Forge command:
- `forge test --match-test testSellerCanDoubleCollectAndStealLaterSaleProceeds -vv`
## Recommended Mitigation
Consume the claim state before transferring tokens (checks-effects-interactions) so a settled listing cannot be collected again.
**Option A (claimed flag)**
```solidity
Listing storage listing = listings[tokenId];
require(!listing.claimed, "already collected");
listing.claimed = true;
uint256 collateral = collateralForMinting[tokenId];
collateralForMinting[tokenId] = 0;
```
**Option B (delete listing)**
```solidity
Listing storage listing = listings[tokenId];
collateralForMinting[tokenId] = 0;
delete listings[tokenId];
```
