Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

State Inconsistency in sellErc20 Function Due to Failed Token Transfer

sauronsol

Description

The sellErc20 function in the TokenDivider contract updates the user's balance and creates a sell order before transferring the tokens. If the token transfer fails (e.g., due to a non-standard ERC20 implementation returning false), the function does not revert, leaving the contract in an inconsistent state where the user's balance is reduced but the tokens were not actually transferred.

Impact

  • Loss of Tokens: Users can lose access to their tokens if the transfer fails, as their balance is reduced without the actual transfer taking place.

  • Permanent Inconsistency: The contract's state becomes permanently inconsistent, with balances not reflecting actual token ownership.

  • Potential for Exploitation: Malicious ERC20 tokens could be used to manipulate the contract's state.

Proof of Concept

function testStateInconsistencyInSellErc20() public {
// Setup: User has ERC20 tokens and approves TokenDivider
// Mock the ERC20.transferFrom to return false
vm.mockCall(
erc20Address,
abi.encodeWithSelector(
IERC20.transferFrom.selector,
SELLER,
address(tokenDivider),
AMOUNT
),
abi.encode(false)
);
// Call sellErc20 - the function doesn't revert despite transfer failure
tokenDivider.sellErc20(address(nft), PRICE, AMOUNT);
// Balance is reduced but tokens weren't transferred
assertEq(tokenDivider.getBalanceOf(SELLER, erc20Address), 0);
// Order is created despite failed transfer
(address orderSeller,,, uint256 orderAmount) = tokenDivider.getUserSellOrder(SELLER, 0);
assertEq(orderAmount, AMOUNT);
assertEq(orderSeller, SELLER);
}

Tools Used

  • Manual code review

  • Foundry for testing

  • Slither static analysis

Recommendations

  1. Follow the Checks-Effects-Interactions pattern:

function sellErc20(address nftPegged, uint256 price, uint256 amount) external {
// ... existing checks ...
// Transfer tokens first
bool success = IERC20(tokenInfo.erc20Address).transferFrom(
msg.sender,
address(this),
amount
);
if (!success) {
revert TokenDivider__TransferFailed();
}
// Update state after successful transfer
balances[msg.sender][tokenInfo.erc20Address] -= amount;
s_userToSellOrders[msg.sender].push(
SellOrder({
seller: msg.sender,
erc20Address: tokenInfo.erc20Address,
price: price,
amount: amount
})
);
emit OrderPublished(amount, msg.sender, nftPegged);
}

  1. Use OpenZeppelin's SafeERC20 library to handle non-standard ERC20 implementations.

  2. Add proper error handling for failed transfers.

Updates

Lead Judging Commences

fishy Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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