80bit collision attack on bid.commitment

[kchalkias]

At the moment, bid commits are truncated to 160 bits: bytes20 bidHash = bytes20(keccak256(abi.encode(nonce, bidValue)));
Bidders control their nonce and bidValue and thus, it is possible to find two bids (one with a large amount and another with a small amount) with the same hash by just offchain retrying/playing with the nonce value.

Then, based on the 2nd highest bid, they can decide if they open the commitment with the large amount (to win the auction item) or the small amount (to get refunded).

Although it's an expensive attack, it's theoretically feasible with today's CPU power. Unfortunately, the attack is also reusable. The current smart-contract design does not apply any domain separation flag in the hash-inputs, which implies that an adversary can find one such collision and reuse it in different auctions. Well, if one reuses commitments between auctions, others can spot this if they do analysis in past transactions for matching commitments, but that's a detail. Including the auctionID in the hash inputs would be preferable as a hygiene solution to avoid such edge cases.

PS: obviously for the attack to make sense, one should consider the energy + resources cost of a 2^80 collision, thus it would only matter in practice for very expensive auctioned items (or reuse many times), but still; cryptographically, the advertised security of the current auction contract is considered to be @80bits, which is not ideal.

To sum up: a straightforward proposal is
a) do not truncate the commitment hashes.
b) add auctionID as hash input to the commitment derivation, to avoid reusing the same commit in different auctions.

[moodlezoup]

Hi Kostas, thank you for flagging this! I will be adding tokenContract, tokenId, and auctionIndex to the hash input for the commitment derivation to prevent reuse, as you suggested. We discussed this a bit internally and given the high cost and relatively low impact of such an attack, we think that we can leave the commitments at 160 bits (at least for now), but will add some explicit disclaimers on this in the contract.