AuctionRunnerFacet Manual Review Findings

ARF-01M: Ineffectual Protection & Bid Refund Re-Entrancy

Description:

The bid code refunds the previous buyer's bid before replacing the bid data with the new one. This permits a re-entrancy attack to unfold whereby the current buyer of an auction is informed of the action. As a result, a complex attack can unfold whereby a secondary auction can steal user funds by creating an auction and a corresponding bid, waiting for a victim's outbid and then during the re-entrancy cancelling the auction thereby refunding the bid to the malicious attacker twice while causing the victim's funds to be permanently lost in the contract. We should note that the attack vector is possible due to the fact that the isContract safe-guard is ineffectual and can be bypassed by the constructor of a contract given that it will yield false for a contract in-creation.

Example:

Copy
115// Make sure we can accept the caller's bid
116require(!AddressUpgradeable.isContract(msg.sender), "Contracts may not bid");
117require(block.timestamp >= auction.start, "Auction hasn't started");
118if ((auction.state != State.Pending) || (auction.clock != Clock.Trigger)) {
119    require(block.timestamp <= endTime, "Auction timer has elapsed");
120}
121require(msg.value >= auction.reserve, "Bid below reserve price");
122
123// If a standing bid exists:
124// - Be sure new bid outbids previous
125// - Give back the previous bidder's money
126if (auction.bid > 0) {
127    require(msg.value >= (auction.bid + getPercentageOf(auction.bid, getMarketController().getOutBidPercentage())), "Bid too small");
128    auction.buyer.transfer(auction.bid);
129    emit BidReturned(consignment.id, auction.buyer, auction.bid);
130}
131
132// Record the new bid
133auction.bid = msg.value;
134auction.buyer = payable(msg.sender);
135getMarketController().setConsignmentPendingPayout(consignment.id, msg.value);

Recommendation:

We advise the Checks-Effects-Interactions (CEI) pattern to be conformed to by applying a pull pattern to bid refunds. If refunds are disbursed immediately, the buyer will be able to outbid the current bidder and generally cause the bid system to be rigged.

Alleviation:

Although the statements were re-ordered to conform to the CEI pattern, the code still performs a regular refund at the end of execution. This allows a smart-contract based bidder to automatically re-bid in the auction. We assume the support of "smart" bidders is intended and as such consider this exhibit dealt with.

ARF-02M: Deprecated Native Asset Transfer

Description:

The transfer member exposed by payable address types has been deprecated as it does not reliably execute and can fail in future updates of the EVM as it forwards a fixed gas stipend which is not compatible with gas cost EIP upgrades such as EIP-2929.

Example:

Copy
128auction.buyer.transfer(auction.bid);

Recommendation:

We advise a safe wrapper library to be utilized instead such as the sendValue function of the Address library by OpenZeppelin which is guaranteed to execute under all circumstances.

Alleviation:

The codebase was updated to utilize an internal sendValueOrCreditAccount function that attempts to send the native asset to the recipient address and in case the action fails for any reason, the user is given a redeemable credit of their native asset portion. This change has alleviated this exhibit in full.