code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Sellers provide both a public key (in the AuctionParameters struct) and an encrypted private key as arguments to SizeSealed#createAuction. The seller's public key is saved in storage, and their encrypted private key is emitted as an event parameter:

createAuction:

function createAuction(
  AuctionParameters calldata auctionParams,
  Timings calldata timings,
  bytes calldata encryptedSellerPrivKey
) external returns (uint256) {
  if (timings.endTimestamp <= block.timestamp) {
    revert InvalidTimestamp();
  }
  if (timings.startTimestamp >= timings.endTimestamp) {
    revert InvalidTimestamp();
  }
  if (timings.endTimestamp > timings.vestingStartTimestamp) {
    revert InvalidTimestamp();
  }
  if (timings.vestingStartTimestamp > timings.vestingEndTimestamp) {
    revert InvalidTimestamp();
  }
  if (timings.cliffPercent > 1e18) {
    revert InvalidCliffPercent();
  }
  // Revert if the min bid is more than the total reserve of the auction
  if (
    FixedPointMathLib.mulDivDown(
      auctionParams.minimumBidQuote,
      type(uint128).max,
      auctionParams.totalBaseAmount
    ) > auctionParams.reserveQuotePerBase
  ) {
    revert InvalidReserve();
  }

  uint256 auctionId = ++currentAuctionId;

  Auction storage a = idToAuction[auctionId];
  a.timings = timings;

  a.data.seller = msg.sender;
  a.data.lowestQuote = type(uint128).max;

  a.params = auctionParams;

  // Passes https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
  // Transfer base tokens to auction contract and check for tax tokens
  uint256 balanceBeforeTransfer = ERC20(auctionParams.baseToken).balanceOf(
    address(this)
  );

  SafeTransferLib.safeTransferFrom(
    ERC20(auctionParams.baseToken),
    msg.sender,
    address(this),
    auctionParams.totalBaseAmount
  );

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

  emit AuctionCreated(
    auctionId,
    msg.sender,
    auctionParams,
    timings,
    encryptedSellerPrivKey
  );

  return auctionId;
}

In order to finalize a completed auction, the seller later calls reveal, revealing their private key and enabling decryption of valid bids encrypted to the seller's public key:

reveal:

function reveal(
  uint256 auctionId,
  uint256 privateKey,
  bytes calldata finalizeData
) external atState(idToAuction[auctionId], States.RevealPeriod) {
  Auction storage a = idToAuction[auctionId];
  if (a.data.seller != msg.sender) {
    revert UnauthorizedCaller();
  }

  ECCMath.Point memory pubKey = ECCMath.publicKey(privateKey);
  if (
    pubKey.x != a.params.pubKey.x ||
    pubKey.y != a.params.pubKey.y ||
    (pubKey.x == 1 && pubKey.y == 1)
  ) {
    revert InvalidPrivateKey();
  }

  a.data.privKey = privateKey;

  emit RevealedKey(auctionId, privateKey);

  if (finalizeData.length != 0) {
    (
      uint256[] memory bidIndices,
      uint128 clearingBase,
      uint128 clearingQuote
    ) = abi.decode(finalizeData, (uint256[], uint128, uint128));
    finalize(auctionId, bidIndices, clearingBase, clearingQuote);
  }
}

However, there is no mechanism preventing sellers from reusing a revealed public-private keypair in future auctions, or preventing one seller from reusing the key revealed by another. If a keypair is accidentally or intentionally reused, sealed bids could be immediately decrypted as soon as they are placed. (Note that the encrypted bid and public key are both emitted in the Bid event).

Impact:

A seller may accidentally reuse their keypair, unintentionally revealing sealed bids.
A malicious third party may manipulate a seller into reusing a previously used keypair, revealing sealed bids.
A malicious seller who wishes to reveal sealed bids may intentionally choose to reuse a keypair.

Severity:

High impact (all bids are unintentionally revealed, defeating the purpose of a private auction), but medium likelihood, since the seller must make a mistake or be manipulated into reusing their keypair.

Recommendation:

Track revealed public keys in a mapping and revert in createAuction if a seller reuses a previously revealed public key.

Test case:

function testReuseKeypair() public {
  (uint256 sellerBeforeQuote, uint256 sellerBeforeBase) = seller.balances();
  uint256 aid = seller.createAuction(
    baseToSell,
    reserveQuotePerBase,
    minimumBidQuote,
    startTime,
    endTime,
    unlockTime,
    unlockEnd,
    cliffPercent
  );
  bidder1.setAuctionId(aid);
  (uint256 buyerBeforeQuote, ) = bidder1.balances();
  bidder1.bidOnAuctionWithSalt(1 ether, 5e6, "hello");
  uint256[] memory bidIndices = new uint[](1);
  bidIndices[0] = 0;
  vm.warp(endTime + 1);
  seller.finalize(bidIndices, 1 ether, 5e6);

  // Seller can create and finalize another auction with the same keypair
  uint256 aid2 = seller.createAuction(
    baseToSell,
    reserveQuotePerBase,
    minimumBidQuote,
    uint32(block.timestamp),
    uint32(block.timestamp + 1 days),
    uint32(block.timestamp + 10 days),
    uint32(block.timestamp + 20 days),
    cliffPercent
  );
  bidder2.setAuctionId(aid2);
  (buyerBeforeQuote, ) = bidder2.balances();
  bidder2.bidOnAuctionWithSalt(1 ether, 5e6, "hello");
  bidIndices = new uint[](1);
  bidIndices[0] = 0;
  vm.warp(block.timestamp + 1 days + 1);
  seller.finalize(bidIndices, 1 ether, 5e6);
}

trust1995 commented 2 years ago

Reusing a previous private key is such a blatant user error , which would not be allowed in the UI, that labeling as High risk is overinflation.

0xean commented 2 years ago

I agree with trust here, definitely overinflated severity.

c4-judge commented 2 years ago

0xean marked the issue as unsatisfactory: Overinflated severity

horsefacts commented 1 year ago

EDIT: After some reflection and two thumbs-down emoji, I'm removing this. (The original comment is still available in the edit history). "Don't comment on your own findings" is a good rule, I think I broke it here, and I don't want to set the precedent that everyone gets to write a dissenting opinion. I still feel strongly about the issues I raised, but will take those conversations to other venues. 🫡

code-423n4 / 2022-11-size-findings

Sellers may reuse revealed keypairs #147

Lines of code

Vulnerability details