Reentrancy in createAuction() function

[code423n4]

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens missing return values and don't fail in case of an unsuccessful transfer. Also, ERC777 tokens could call the tokens receivers during the transfer. If baseToken would be such a token that combines both properties - this could lead to a reentrancy attack on createAuction() function.

Proof of Concept

1. Attacker calls createAuction() from malicious contract with totalBaseAmount of 100. Malicious contract balance of baseToken is 100.
2. Inside createAuction() call contract saves balance before the transfer, which is 0 at that moment:

   src/SizeSealed.sol:96    uint256 balanceBeforeTransfer = ERC20(auctionParams.baseToken).balanceOf(address(this));

3. Size contract calls safeTransferFrom on baseToken:

   src/SizeSealed.sol:98    SafeTransferLib.safeTransferFrom(
   src/SizeSealed.sol:99       ERC20(auctionParams.baseToken), msg.sender, address(this), auctionParams.totalBaseAmount
   src/SizeSealed.sol:100   ); 

4. Base token call attacker malicious contract, which reenters Size contract with the same createAuction() call, successfully creates new auction with totalBaseAmount of 100.
5. Second transfer of 100 tokens from the malicious contract (after the first inside hook call) fails since the malicious contract had only 100 tokens, but transaction does not revert due to the token transfer architecture.
6. Balance of Size contract checks again and it's 100 now (due to successful transfer inside hook call) so it passes the final check:

   src/SizeSealed.sol:102  uint256 balanceAfterTransfer = ERC20(auctionParams.baseToken).balanceOf(address(this));

7. There are two created auctions now, each with totalBaseAmount of 100 base tokens, while the contract has only 100 on its balance.

Recommended Mitigation Steps

Add a reentrancy guard modifier on createAuction() function.

[trust1995]

I believe this finding is invalid because the second transferFrom would fail and the contract would revert. Proof here

[0xean]

Some ERC20 tokens missing return values and don't fail in case of an unsuccessful transfer.

this is what safeTransfer is meant to guard against. Adding a re-entrancy modifier isn't a bad suggestions, but would be QA.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof