[M-01] Auctions Using FOT, burn rates, rebasing erc20s will revert on claim
Summary
The documentation states that auctions should support tokens with additional fees (Fee on Transfer - FOT), with users covering the cost. However, when using such tokens in AuctionFactory::createAuction, the factory automatically transfers totalTokens to the auction address. Since FOT tokens deduct the fee from the received amount, the auction address ends up holding fewer tokens than expected. This discrepancy will cause a revert when the last bidder calls FjordAuction::claimTokens or when the owner calls auctionEnd if no bids were placed.
Vulnerability Details
In the createAuction function, the totalTokens amount is transferred to the new auction contract. However, for FOT tokens, the actual number of tokens received by the auction contract is less than the totalTokens due to the transfer fee.
function createAuction(
address auctionToken,
uint256 biddingTime,
uint256 totalTokens,
bytes32 salt
) external onlyOwner returns (address) {
address auctionAddress = address(
new FjordAuction{salt: salt}(
fjordPoints,
auctionToken,
biddingTime,
totalTokens
)
);
@> IERC20(auctionToken).transferFrom(
@> msg.sender,
@> auctionAddress,
@> totalTokens
@> );
emit AuctionCreated(auctionAddress);
return auctionAddress;
}
The auction contract records totalTokens, but the actual balance will be lower.
constructor(
address _fjordPoints,
address _auctionToken,
uint256 _biddingTime,
uint256 _totalTokens
) {
if (_fjordPoints == address(0)) {
revert InvalidFjordPointsAddress();
}
if (_auctionToken == address(0)) {
revert InvalidAuctionTokenAddress();
}
fjordPoints = ERC20Burnable(_fjordPoints);
auctionToken = IERC20(_auctionToken);
owner = msg.sender;
auctionEndTime = block.timestamp.add(_biddingTime);
totalTokens = _totalTokens;
}
This mismatch leads to a revert during token claims or auction end when the contract tries to transfer more tokens than it holds.
function claimTokens() external {
if (!ended) {
revert AuctionNotYetEnded();
}
uint256 userBids = bids[msg.sender];
if (userBids == 0) {
revert NoTokensToClaim();
}
@> uint256 claimable = userBids.mul(multiplier).div(PRECISION_18);
bids[msg.sender] = 0;
auctionToken.transfer(msg.sender, claimable);
emit TokensClaimed(msg.sender, claimable);
}
Impact
If there are bidders, the last user's claimTokens function will revert. If no bids are placed, the auctionEnd function will revert when called by the owner. Although the problem can be fixed by the owner or user depositing the missing amount directly, it still creates an unnecessary complication and can be prevented by verifying the actual amount of transferred tokens.
Proof of Concept
Add the following test to the existing auction.t.sol test suite.
Note that we are using the d-xo/weird-erc20 repository's implementation for Fee on Transfer (FOT) tokens. Since the tests don't use the factory, the factory was added to the existing test file. There is also a commented-out part of the test that works for the case where there are no bidders—uncomment it to test for that scenario.
import "forge-std/Test.sol";
import "src/FjordAuction.sol";
import {AuctionFactory} from "src/FjordAuctionFactory.sol";
import {ERC20BurnableMock} from "../mocks/ERC20BurnableMock.sol";
import {SafeMath} from "lib/openzeppelin-contracts/contracts/utils/math/SafeMath.sol";
import {TransferFeeToken} from "lib/weird-erc20/src/TransferFee.sol";
contract TestAuction is Test {
using SafeMath for uint256;
FjordAuction public auction;
ERC20BurnableMock public fjordPoints;
ERC20BurnableMock public auctionToken;
address public owner = address(0x1);
uint256 public biddingTime = 1 weeks;
uint256 public totalTokens = 1000 ether;
TransferFeeToken public fotAcutionToken;
AuctionFactory factory;
function setUp() public {
fjordPoints = new ERC20BurnableMock("FjordPoints", "fjoPTS");
auctionToken = new ERC20BurnableMock("AuctionToken", "AUCT");
fotAcutionToken = new TransferFeeToken(100_000 ether, 1 ether);
auction = new FjordAuction(
address(fjordPoints),
address(auctionToken),
biddingTime,
totalTokens
);
factory = new AuctionFactory(address(fjordPoints));
deal(address(auctionToken), address(auction), totalTokens);
}
function testFactory() public {
fotAcutionToken.approve(address(factory), totalTokens);
auction = FjordAuction(
factory.createAuction(
address(fotAcutionToken),
biddingTime,
totalTokens,
keccak256(abi.encode("123456"))
)
);
vm.assertEq(
fotAcutionToken.balanceOf(address(auction)),
totalTokens - 1 ether
);
address bidder = address(0x2);
address bidder2 = address(0x3);
uint256 bidAmount = 100 ether;
deal(address(fjordPoints), bidder, bidAmount);
deal(address(fjordPoints), bidder2, bidAmount);
vm.startPrank(bidder);
fjordPoints.approve(address(auction), bidAmount);
auction.bid(bidAmount);
vm.stopPrank();
vm.startPrank(bidder2);
fjordPoints.approve(address(auction), bidAmount);
auction.bid(bidAmount);
vm.stopPrank();
vm.roll(block.number + 1);
vm.warp(block.timestamp + 2 weeks);
auction.auctionEnd();
vm.startPrank(bidder);
auction.claimTokens();
vm.expectRevert("insufficient-balance");
vm.startPrank(bidder2);
auction.claimTokens();
}
}
Tools Used
Manual Review
Recommendations
A straightforward solution is to check the contract’s actual token balance during the auctionEnd function instead of relying on the recorded totalTokens. This approach ensures the auction can fully support FOT tokens and avoids reverts caused by balance discrepancies. Additionally, using the actual balance during the calculation of the multiplier ensures any extra tokens sent to the contract are correctly accounted for and rewarded to users. also in the implementation below a instead of loading totalTokens from storage so manytimes a _totalTokens memory variable is introduced that will be used to reduce the gas cost.
function auctionEnd() external {
if (block.timestamp < auctionEndTime) {
revert AuctionNotYetEnded();
}
if (ended) {
revert AuctionEndAlreadyCalled();
}
+ uint256 _totalTokens = totalTokens; //use a memory variable to reduce gas usage!
+ uint256 actualBalance = auctionToken.balanceOf(address(this));
+ if (_totalTokens != actualBalance) {
+ _totalTokens = actualBalance;
+ }
ended = true;
- emit AuctionEnded(totalBids, totalTokens);
+ emit AuctionEnded(totalBids, _totalTokens);
if (totalBids == 0) {
- auctionToken.transfer(owner, totalTokens);
+ auctionToken.transfer(owner, _totalTokens);
return;
}
- multiplier = totalTokens.mul(PRECISION_18).div(totalBids);
+ multiplier = _totalTokens.mul(PRECISION_18).div(totalBids);
// Burn the FjordPoints held by the contract
uint256 pointsToBurn = fjordPoints.balanceOf(address(this));
fjordPoints.burn(pointsToBurn);
}
