SIZE contest - cryptonue's results

Lines of code

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

Vulnerability details

Impact

Draining baseToken from SizeSealed contract by calling finalize function multiple times

Proof of Concept

The finalize() function can be called multiple times by providing clearingQuote to type(uint128).max.

Currently inside finalize() function there is no check condition if an auction already call finalize(). The only close check condition to this is the atState modifier which will prevent calling this finalize() again because it will move from RevealPeriod to Finalized state (if the clearingQuote is for some number (not type(uint128).max value)

File: SizeSealed.sol
217:     function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote)
218:         public
219:         atState(idToAuction[auctionId], States.RevealPeriod)
220:     {
221:         Auction storage a = idToAuction[auctionId];
222:         uint256 sellerPriv = a.data.privKey;
223:         if (sellerPriv == 0) {
224:             revert InvalidPrivateKey();
225:         }
226: 
227:         if (bidIndices.length != a.bids.length) {
228:             revert InvalidCalldata();
229:         }
230: 
231:         FinalizeData memory data;
232:         data.reserveQuotePerBase = a.params.reserveQuotePerBase;
233:         data.totalBaseAmount = a.params.totalBaseAmount;
234:         data.previousQuotePerBase = type(uint256).max;
235: 
236:         // Last filled bid is the clearing price
237:         a.data.lowestBase = clearingBase;
238:         a.data.lowestQuote = clearingQuote;
239: 
240:         // Bitmap of all the bid indices that have been processed
241:         uint256[] memory seenBidMap = new uint256[]((bidIndices.length/256)+1);
242: 
243:         // Fill orders from highest price to lowest price
244:         for (uint256 i; i < bidIndices.length; i++) {
245:             uint256 bidIndex = bidIndices[i];
246:             EncryptedBid storage b = a.bids[bidIndex];
247: 
248:             // Verify this bid index hasn't been seen before
249:             uint256 bitmapIndex = bidIndex / 256;
250:             uint256 bitMap = seenBidMap[bitmapIndex];
251:             uint256 indexBit = 1 << (bidIndex % 256);
252:             if (bitMap & indexBit == 1) revert InvalidState();
253:             seenBidMap[bitmapIndex] = bitMap | indexBit;
254: 
255:             // G^k1^k2 == G^k2^k1
256:             ECCMath.Point memory sharedPoint = ECCMath.ecMul(b.pubKey, sellerPriv);
257:             // If the bidder public key isn't on the bn128 curve
258:             if (sharedPoint.x == 1 && sharedPoint.y == 1) continue;
259: 
260:             bytes32 decryptedMessage = ECCMath.decryptMessage(sharedPoint, b.encryptedMessage);
261:             // If the bidder didn't faithfully submit commitment or pubkey
262:             // Or the bid was cancelled
263:             if (computeCommitment(decryptedMessage) != b.commitment) continue;
264: 
265:             // First 128 bits are the base amount, last are random salt
266:             uint128 baseAmount = uint128(uint256(decryptedMessage >> 128));
267: 
268:             // Require that bids are passed in descending price
269:             uint256 quotePerBase = FixedPointMathLib.mulDivDown(b.quoteAmount, type(uint128).max, baseAmount);
270:             if (quotePerBase >= data.previousQuotePerBase) {
271:                 // If last bid was the same price, make sure we filled the earliest bid first
272:                 if (quotePerBase == data.previousQuotePerBase) {
273:                     if (data.previousIndex > bidIndex) revert InvalidSorting();
274:                 } else {
275:                     revert InvalidSorting();
276:                 }
277:             }
278: 
279:             // Only fill if above reserve price
280:             if (quotePerBase < data.reserveQuotePerBase) continue;
281: 
282:             // Auction has been fully filled
283:             if (data.filledBase == data.totalBaseAmount) continue;
284: 
285:             data.previousQuotePerBase = quotePerBase;
286:             data.previousIndex = bidIndex;
287: 
288:             // Fill the remaining unfilled base amount
289:             if (data.filledBase + baseAmount > data.totalBaseAmount) {
290:                 baseAmount = data.totalBaseAmount - data.filledBase;
291:             }
292: 
293:             b.filledBaseAmount = baseAmount;
294:             data.filledBase += baseAmount;
295:         }
296: 
297:         if (data.previousQuotePerBase != FixedPointMathLib.mulDivDown(clearingQuote, type(uint128).max, clearingBase)) {
298:             revert InvalidCalldata();
299:         }
300: 
301:         // seenBidMap[0:len-1] should be full
302:         for (uint256 i; i < seenBidMap.length - 1; i++) {
303:             if (seenBidMap[i] != type(uint256).max) {
304:                 revert InvalidState();
305:             }
306:         }
307: 
308:         // seenBidMap[-1] should only have the last N bits set
309:         if (seenBidMap[seenBidMap.length - 1] != (1 << (bidIndices.length % 256)) - 1) {
310:             revert InvalidState();
311:         }
312: 
313:         if (data.filledBase > data.totalBaseAmount) {
314:             revert InvalidState();
315:         }
316: 
317:         // Transfer the left over baseToken
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:         }
323: 
324:         // Calculate quote amount based on clearing price
325:         uint256 filledQuote = FixedPointMathLib.mulDivDown(clearingQuote, data.filledBase, clearingBase);
326: 
327:         SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), a.data.seller, filledQuote);
328: 
329:         emit AuctionFinalized(auctionId, bidIndices, data.filledBase, filledQuote);
330:     }

if this call being executed multiple times it can drained the contract's baseToken

Tools Used

Manual analysis

Recommended Mitigation Steps

check condition if an auction is already called finalize() function, (perhaps by storing to the auction storage if it's being finalized), if so then revert

   Auction storage a = idToAuction[auctionId];
   if(a.finalized) revert AuctionAlreadyFinalized()
   else a.finalized = true

Lines of code

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

Vulnerability details

Impact

Denial of service from malicious bidder by filling up the bid index up until 1000

Proof of Concept

Initially inside bid() function there is a check condition if bidIndex >= 1000 then it will revert, so this will limit the size of bid created by bidder to prevent for DoS attack (if there is unlimited index, then it can easily dos-ed)

File: SizeSealed.sol
155:         uint256 bidIndex = a.bids.length;
156:         // Max of 1000 bids on an auction to prevent DOS
157:         if (bidIndex >= 1000) {
158:             revert InvalidState();
159:         }

Unfortunately, this condition is not enough, because

1. There is a function to cancel bid cancelBid() which return user's asset when they bid
2. The cancelled bid index is still exist

with this situation, any malicious bidder can just bid() and cancelBid() up until 1000 without losing their asset, (only need a gas fee).

Tools Used

Manual Analysis

Recommended Mitigation Steps

If needed, when bidder cancelled their bid, the bid index should be removed

[NC] Missing indexed in event

Indexed events are easier and faster to find in the transaction log. So it's better to use indexed in event. the ISizeSealed.sol events aren't using index

File: ISizeSealed.sol
096: 
097:     event AuctionCreated(
098:         uint256 auctionId, address seller, AuctionParameters params, Timings timings, bytes encryptedPrivKey
099:     );
100: 
101:     event AuctionCancelled(uint256 auctionId);
102: 
103:     event Bid(
104:         address sender,
105:         uint256 auctionId,
106:         uint256 bidIndex,
107:         uint128 quoteAmount,
108:         bytes32 commitment,
109:         ECCMath.Point pubKey,
110:         bytes32 encryptedMessage,
111:         bytes encryptedPrivateKey
112:     );
113: 
114:     event BidCancelled(uint256 auctionId, uint256 bidIndex);
115: 
116:     event RevealedKey(uint256 auctionId, uint256 privateKey);
117: 
118:     event AuctionFinalized(uint256 auctionId, uint256[] bidIndices, uint256 filledBase, uint256 filledQuote);
119: 
120:     event BidRefund(uint256 auctionId, uint256 bidIndex);
121: 
122:     event Withdrawal(uint256 auctionId, uint256 bidIndex, uint256 withdrawAmount, uint256 remainingAmount);

[NC] Design of bidder flow is not efficient

Currently when a user bid X amount, they bid with sending their token also X amount. At some point, he want to bid a higher amount Y, (which Y > X), so he need to send Y amount. To make it efficient and use smaller index, if a user want to increase their bid, they can just update the previous bid value. In this case if user want to bid Y, they only need to send Y-X amount.

[NC] CONSTANTS SHOULD BE DEFINED RATHER THAN USING MAGIC NUMBERS

File: SizeSealed.sol
157:         if (bidIndex >= 1000) {
158:             revert InvalidState();
159:         }

[L] AVOID NESTED IF BLOCKS

For better readability and analysis it is better to avoid nested if blocks. Here is an example:

File: SizeSealed.sol
132:         if (a.params.merkleRoot != bytes32(0)) {
133:             bytes32 leaf = keccak256(abi.encodePacked(msg.sender));
134:             if (!MerkleProofLib.verify(proof, a.params.merkleRoot, leaf)) {
135:                 revert InvalidProof();
136:             }
137:         }

can be rewrite as:

        if (
            a.params.merkleRoot != bytes32(0) &&
            !MerkleProofLib.verify(
                proof,
                a.params.merkleRoot,
                keccak256(abi.encodePacked(msg.sender))
            )
        ) {
            revert InvalidProof();
        }