claimAuction() revert, leading to permanent loss of auction bid fund.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L112

Vulnerability details

Impact

Attacker can participate auction through a smart contract that did not implement ERC721Receiver, then claimAuction() will revert with error ERC721: transfer to non ERC721Receiver implementer.

ALL the auction bid fund will be permanently loss.

Proof of Concept

• Run forge init in project root

• setup deployer script and test file:
  Gist link to all files: link

Filepath: 
- test/mock/Attacker.sol
- script/DeployProtocol.s.sol
- test/AuctionDemoTest.t.sol

• Run the test:

forge test --mt testMintAndAuction -vv

• Which will yields the following output:
  Assertion failure indicating fund failed to be refunded to participating user.

[⠢] Compiling...
No files changed, compilation skipped

Running 1 test for test/AuctionDemoTest.t.sol:AuctionDemoTest
[FAIL. Reason: Assertion failed.] testMintAndAuction() (gas: 1626277)
Logs:
  Error: a == b not satisfied [uint]
        Left: 9000000000000000000
       Right: 10000000000000000000
  Error: a == b not satisfied [uint]
        Left: 8000000000000000000
       Right: 10000000000000000000

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.30ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/AuctionDemoTest.t.sol:AuctionDemoTest
[FAIL. Reason: Assertion failed.] testMintAndAuction() (gas: 1626277)

Tools Used

Manual Code Review, Forge

Recommended Mitigation Steps

1. The protocol can implement a check if participating address is contract and has ERC721Receiver implemented.

     function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

+        if (isContract(msg.sender)) {
+            // The address is a contract, now check if it supports ERC721Receiver
+           ERC165 erc165 = ERC165(msg.sender);
+            require(erc165.supportsInterface(0x150b7a02), "Contract did not support ERC721Receiver"); // ERC721Receiver's interface ID
+        }

        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

+    function isContract(address _address) public view returns (bool) {
+        uint256 size;
+        assembly {
+            size := extcodesize(_address)
+        }
+        return size > 0;
+    }

2. The participating contract can still be selfdestruct or bypass extcodesize() check and lead to same issue, so it is recommended to implement the withdrawal pattern for participant refund and winner claiming. This lays the burden of proof on the refund/token receiver.

function auctionRefundWithdraw(uint256 _tokenid) public {
    // if auctionEnded && !claimed && msg.sender == participating user 
    // participating user withdrawing their OWN refund only
}

function auctionWinnerClaim(uint256 _tokenid) public {
    // if auctionEnded && !claimed && msg.sender == winner/highest bidder
    // auction Winner claiming their token
}

3. A separate withdrawal function for the protocol owner/admin to withdraw the bid fees after an auction has ended, regardless if the winner has claim their token or not.

function auctionProtocolFeeWithdraw(uint256 _tokenid) public{
    // if auctionEnded && !claimed && msg.sender == admin
    // protocol admin withdraw the winning auction bid
}

Assessed type

ETH-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1759

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)