80bit collision attack on bid.commitment #2
Description
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.