SIZE contest - TomJ's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L55-L56 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/interfaces/ISizeSealed.sol#L77-L78

Vulnerability details

Impact

Tokens such as Aave aTokens are rebasing token where its token amount will increase over time. Since SizeSealed contract reference deposited token amount from user input, any token amount that increased over time will not be referenced which means increased token amount will be locked in the contract forever. Also since SizeSealed contract has no limitation for what token can be used as baseToken and quoteToken, this issue falls for both baseToken and quoteToken.

aTokens reference: https://edge.app/blog/company-news/interest-bearing-tokens-in-edge-atokens-ctokens/

Proof of Concept

Any token can be specified for both baseToken and quoteToken at createAuction function. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L55-L56 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/interfaces/ISizeSealed.sol#L77-L78

1. baseToken

createAuction function: Auction seller sending its specified basetoken amount to SizeSealed contract. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L56 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/interfaces/ISizeSealed.sol#L80 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L98-L100

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

finalize function: When token are sent back to seller, the token amount is referenced from the totalBaseAmount which was specified by the seller when executing createAuction function, meaning that any increased token amount is ignored. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L217-322

217:    function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote)
...
221:        Auction storage a = idToAuction[auctionId];
...
233:        data.totalBaseAmount = a.params.totalBaseAmount;
...
318:        if (data.totalBaseAmount != data.filledBase) {
319:            uint128 unsoldBase = data.totalBaseAmount - data.filledBase;
320:            a.params.totalBaseAmount = data.filledBase;
321:            SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), a.data.seller, unsoldBase);
322:        }

cancelAuction function: Same as finalize function explained above. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L391-L392 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L409

391:    function cancelAuction(uint256 auctionId) external {
392:        Auction storage a = idToAuction[auctionId];
...
409:        SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), msg.sender, a.params.totalBaseAmount);

2. quoteToken

bid function: Bidder sending its specified quoteAmount amount to SizeSealed contract. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L124 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L163

122:    function bid(
124:        uint128 quoteAmount,
...
163:        SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

refund function: When token are sent back to bidder, the token amount is referenced from the quoteAmount which was specified by the bidder when executing bid function, meaning that any increased token amount is ignored. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L336-L338 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L351

336:    function refund(uint256 auctionId, uint256 bidIndex) external atState(idToAuction[auctionId], States.Finalized) {
337:        Auction storage a = idToAuction[auctionId];
338:        EncryptedBid storage b = a.bids[bidIndex];
...
351:        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

withdraw function: Same as refund function explained above. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L376-L381

376:        if (b.quoteAmount != 0) {
377:            uint256 quoteBought = FixedPointMathLib.mulDivDown(baseAmount, a.data.lowestQuote, a.data.lowestBase);
378:            uint256 refundedQuote = b.quoteAmount - quoteBought;
379:            b.quoteAmount = 0;
380:
381:            SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, refundedQuote);

cancelBid function: Same as refund function explained above. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L439

439:        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add whitelist logic to limit what kind of token is allowed to set as baseToken and quoteToken or track total amount currently deposited and allow seller/bidder to withdraw token amount that increased over time.

Lines of code

https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L156-L159

Vulnerability details

Impact

Malicious user can easily DOS any auction by executing bid function 1000 times, since that is the cap of bid per auction according to the contract logic of bid function. The bid function is allowing attacker to easily execute bid function 1000 times due to following 2 factors.

1. bid function allows same address to bid multiple times
2. bid function does not check whether function caller is contract or not

Since function caller can be a contract and same address, attacker can create contract that execute bid function 1000 times and easily DOS any auction, preventing from anyone else to bid in that auction, which is not ideal for auction seller.

Proof of Concept

No one can bid anymore once auction reach 1000 bid times. https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L156-L159

156:        // Max of 1000 bids on an auction to prevent DOS
157:        if (bidIndex >= 1000) {
158:            revert InvalidState();
159:        }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add logic in the bid function to prevent 2 factors explained above. Since 2nd logic explained below has some drawbacks, probably implementing only 1st one is sufficient enough.

1. Prevent same address to bid multiple times

Add a mapping where it stores auctionId to address to uint so that same address can't bid multiple times per auction.

mapping(uint256 => mapping(address => uint256)) internal bitTimesPerAuction;
...
    function bid(
...
        if (bitTimesPerAuction[auctionId][msg.sender] != 0) {
            revert UnauthorizedCaller();
        }
        bitTimesPerAuction[auctionId][msg.sender] = 1;

If it is not ideal for allowing user to bid only once, make a cap of how many times same address can bid in each auction. (below example allowing same address to bid up to 5 times per auction)

mapping(uint256 => mapping(address => uint256)) internal bitTimesPerAuction;
...
    function bid(
...
        if (bitTimesPerAuction[auctionId][msg.sender] > 4) {
            revert UnauthorizedCaller();
        }
        bitTimesPerAuction[auctionId][msg.sender] += 1;

2. Check whether function caller is contract or not

Use openzeppelin isContract function of Address library to check whether contract caller is a contract or not. However this has drawback since this can be circumvented by calling from a contract constructor.

Table of Contents

• Should Use Unchecked Block where Over/Underflow is not Possible
• Defined Variables Used Only Once
• X = X + Y costs less gass than X += Y
• Internal Function Called Only Once can be Inlined
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

 

Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

PoC

1. finalize() of SizeSealed.sol

Wrap line 319 with unchecked since underflow is not possible due to line 313-315 check
313:        if (data.filledBase > data.totalBaseAmount) {
314:            revert InvalidState();
315:        }
319:            uint128 unsoldBase = data.totalBaseAmount - data.filledBase;

https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L313-L319

Mitigation

Wrap the code with uncheck like described in above PoC.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

1. Remove leaf variable of bid() of SizeSealed.sol https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L133-L134 Mitigation: Delete line 133 and replace line 134 with below code

            if (!MerkleProofLib.verify(proof, a.params.merkleRoot, keccak256(abi.encodePacked(msg.sender)))) {

2. Remove unsoldBase variable of finalize() of SizeSealed.sol https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L319-L321 Mitigation: Delete line 319 and replace line 321 with below code

            SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), a.data.seller, data.totalBaseAmount - data.filledBase);

3. Remove refundedQuote variable of withdraw() of SizeSealed.sol https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L378-L381 Mitigation: Delete line 378 and replace line 381 with below code

            SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount - quoteBought);

4. Remove data variable of ecMul() of ECCMath.sol https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/util/ECCMath.sol#L26-L28 Mitigation: Delete line 26 and replace line 28 with below code

        (bool res, bytes memory ret) = address(0x07).staticcall{gas: 6000}(abi.encode(point, scalar));

5. Remove sharedKey variable of encryptMessage() of ECCMath.sol https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/util/ECCMath.sol#L43-L44 Mitigation: Delete line 43 and replace line 44 with below code

        encryptedMessage = message ^ hashPoint(sharedPoint);

Mitigation

Do not define variable that is used only once. Details are listed on above PoC.

 

X = X + Y costs less gass than X += Y

Issue

X = X + Y costs less gass than X += Y for state variables

PoC

Total of 1 instance found.

SizeSealed.sol:373:        b.baseWithdrawn += baseTokensAvailable;

Change to

SizeSealed.sol:373:        b.baseWithdrawn = b.baseWithdrawn + baseTokensAvailable;

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 1 instance found.

1. tokensAvailableAtTime() of CommonTokenMath.sol This function called only once at SizeSealed.sol(line 457) https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/util/CommonTokenMath.sol#L47 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L457

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size. Therefore it is more gas saving to use 32 bytes unless it is used in a struct or state variable in order to reduce storage slot.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 13 instances found.

CommonTokenMath.sol:48:        uint32 vestingStart,
CommonTokenMath.sol:49:        uint32 vestingEnd,
CommonTokenMath.sol:50:        uint32 currentTime,
CommonTokenMath.sol:51:        uint128 cliffPercent,
CommonTokenMath.sol:52:        uint128 baseAmount
CommonTokenMath.sol:53:    ) internal pure returns (uint128) {
SizeSealed.sol:124:        uint128 quoteAmount,
SizeSealed.sol:319:            uint128 unsoldBase = data.totalBaseAmount - data.filledBase;
SizeSealed.sol:365:        uint128 baseAmount = b.filledBaseAmount;
SizeSealed.sol:370:        uint128 baseTokensAvailable = tokensAvailableForWithdrawal(auctionId, baseAmount);
SizeSealed.sol:451:    function tokensAvailableForWithdrawal(uint256 auctionId, uint128 baseAmount)
SizeSealed.sol:454:        returns (uint128 tokensAvailable)
SizeSealed.sol:470:    function computeMessage(uint128 baseAmount, bytes16 salt) external pure returns (bytes32) {

Mitigation

I suggest using uint256 instead of anything smaller and downcast where needed.