SIZE contest - brgltd's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L163 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L327 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L351 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L381 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L439

Vulnerability details

Impact

Not checking for token existence is a know issue for Solmate. This can cause unexpected contract functionality for transfers implemented in SizeSealed. Note that this might not be a problem for baseToken due to the check implemented in L103. However, this can lead to issues for quoteToken.

Proof of Concept

• User calls createAuction() with a quoteToken in which the address doesn't contain code
• Calls to bid(), finalize(), refund(), withdraw() and cancelBid() will not revert and will result in silent failures.

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

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

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

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

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

Recommended Mitigation Steps

Check if baseToken, and most importantly if quoteToken, contain code when creating an auction.

diff --git a/src/SizeSealed.sol b/src/SizeSealed.sol
--- a/src/SizeSealed.sol
+++ b/src/SizeSealed.sol
@@ -81,6 +81,13 @@ contract SizeSealed is ISizeSealed {
             revert InvalidReserve();
         }

+        if (
+            auctionParams.baseToken.code.length == 0 ||
+            auctionParams.quoteToken.code.length == 0
+        ) {
+            revert InvalidToken();
+        }
+
         uint256 auctionId = ++currentAuctionId;

         Auction storage a = idToAuction[auctionId];

[01] Lack of address(0) validation when creating an auction

Consider adding a check to validate auctionParams.baseToken and auctionParams.quoteToken are not address zero.

If these variables get configured with address zero, it will result in unexpected behavior for the contract.

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

https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L77-L78

[02] Missing natspec/documentation

Consider adding natspec on all functions to improve documentation.

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

[03] Order of functions

The solidity documentation recommends the following order for functions:

constructor receive function (if exists) fallback function (if exists) external public internal private

The instances bellow shows one public function between two external functions.

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

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

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

[04] Inconsistent use of named return variables

Some functions return named variables, others return explicit values.

Following function returns an explicit value

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

Following function returns a named variable

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

Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.