SIZE contest - minhtrng's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/79aa9c01987e57a760521acecfe81b28eab3b313/src/SizeSealed.sol#L150-L163

Vulnerability details

Impact

The code contains logic to prevent the usage of fee-on-transfer tokens as the baseToken. No such logic is present for the quoteToken which can lead to faulty accounting and tokens getting stuck.

Proof of Concept

The function createAuction checks if the baseToken has fees-on-transfer and reverts if it does:

uint256 balanceAfterTransfer = ERC20(auctionParams.baseToken).balanceOf(address(this));
if (balanceAfterTransfer - balanceBeforeTransfer != auctionParams.totalBaseAmount) {
    revert UnexpectedBalanceChange();
}

This check is not present in the function bid where the quoteToken is transferred into the contract. During refund, cancelAuction or finalize this can lead to reversions and tokens being locked, due to the contract holding less quoteToken than the code assumes:

ebid.quoteAmount = quoteAmount; //assumes full quoteAmount will end up getting transferred to contract
...
// no checks here however
SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

Tools Used

Manual Review

Recommended Mitigation Steps

Perform the same check for unexpected balance changes in bid as is done in createAuction, and also revert here if fee-on-transfer-tokens are not meant to be used.

Lines of code

https://github.com/code-423n4/2022-11-size/blob/79aa9c01987e57a760521acecfe81b28eab3b313/src/SizeSealed.sol#L157-L159

Vulnerability details

Impact

The implementation for bidding has a DOS prevention mechanism, which can be abused to disable others from participating in auctions and being able to win the auction at the reserve price.

Proof of Concept

The bid function contains the following logic:

if (bidIndex >= 1000) {
    revert InvalidState();
}

While the purpose of having this logic to avoid a DOS during finalize is clear and reasonable, the drawback is that someone could create 1000 bids in an automated way to occupy all bid slots and be guaranteed to win the auction at the reserveQuotePerBase auction. The exploiter could also cancel most of them right after creation to limit the amount of quoteTokens he needs to perform the attack.

A savvy auction creator would of course be able to monitor this behavior and decide not to finalize and re-do the auction, which would essentially turn into cat-mouse-game scenario, where the exploiter is disadvantaged in terms of gas costs. Depending on the specific network fees and the economical incentive of winning the auction, this can still be a practical issue.

Tools Used

Manual review

Recommended Mitigation Steps

There is no easy fix that does not require an in-depth rework of the current implementation. An idea would be to enable partial reveals, with the requirement that the partial reveals cover all bids. This would however increase the complexity of necessary accounting and bookkeeping.