Bid does not account for possible ERC20 transfer fee

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol/#L122

Vulnerability details

function bid(
        uint256 auctionId,
        uint128 quoteAmount...{
    ebid.quoteAmount = quoteAmount;
}

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

The STA transfer fee was used to drain $500k from several balancer pools (more details).

example: TransferFee.sol

Impact

Since the bid assumes all tokens were sent to the contract, then the ebid.quoteAmount = quoteAmount; will be invalid for tokens with transfer fees. It will be more than the actual tokens received.

One failure in this case will be cancelBid. It will fail because it wants to transfer back ebid.quoteAmount, but there aren’t enough tokens in the contract.

See the notes below for other failing tests.

Mitigation

Check the quoteToken balance before and after token transfer in bid, and use that difference as the ebid.quoteAmount

notes

List of failing tests if there is a transfer fee

Failing tests:
Encountered 7 failing tests in src/test/SizeSealed.t.sol:SizeSealedTest
[FAIL. Reason: TRANSFER_FAILED] testAuctionFinalizePartial() (gas: 1053974)
[FAIL. Reason: TRANSFER_FAILED] testAuctionOneBidFinalise() (gas: 683269)
[FAIL. Reason: TRANSFER_FAILED] testAuctionRefundLostBidder() (gas: 973659)
[FAIL. Reason: TRANSFER_FAILED] testCancelAuctionAfterFinalization() (gas: 671521)
[FAIL. Reason: TRANSFER_FAILED] testCancelBidAfterFinalize() (gas: 671282)
[FAIL. Reason: TRANSFER_FAILED] testCancelBidDuringVoidedNoFinalize() (gas: 624035)
[FAIL. Reason: TRANSFER_FAILED] testCancelSingleBid() (gas: 630448)

POC

Use this MockERC20 as the quoteToken in SizeSealed.t.sol. Verify that the above tests fail.

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.17;

import {ERC20} from "solmate/tokens/ERC20.sol";

contract MockERC20WithFee is ERC20 {
    event Transfer(uint from, uint to, uint fee);
    uint constant fee = 1000;

    constructor(string memory name, string memory symbol, uint8 decimals) ERC20(name, symbol, decimals) {}

    function transferFrom(address src, address dst, uint wad) override public returns (bool) {
        require(balanceOf[src] >= wad, "insufficient-balance");
        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            require(allowance[src][msg.sender] >= wad, "insufficient-allowance");
            allowance[src][msg.sender] = sub(allowance[src][msg.sender], wad);
        }

        balanceOf[src] = sub(balanceOf[src], wad);
        balanceOf[dst] = add(balanceOf[dst], sub(wad, fee));
        balanceOf[address(0)] = add(balanceOf[address(0)], fee);

        emit Transfer(src, dst, sub(wad, fee));
        emit Transfer(src, address(0), fee);

        return true;
    }

    function add(uint256 a, uint256 b) internal pure returns (uint256) {
        return a + b;
    }

    function sub(uint256 a, uint256 b) internal pure returns (uint256) {
        return a - b;
    }

    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

[trust1995]

Dup of #255

[c4-judge]

0xean marked the issue as duplicate

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 2 (Med Risk)