NextGen - 00decree's results

Lines of code

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

Vulnerability details

Impact

Since the contract allow multiple active bid, an Attacker can place a minimum bid and another arbitrary high bid to prevent other user from participating in the auction. Then right before the auction ends, the Attacker can cancel the highest bid and win the auction with the minimum bid, essentially preventing any competition.

Proof of Concept

• Run forge init in project root

• setup deployer script and test file: Gist link: link

Filepath: 
- script/DeployProtocol2.s.sol
- test/AuctionDemoTest2.t.sol

• Run the test:

forge test --mt testAttackerMultipleActiveBid -vv

• Which will yields the following output: Assertion will pass, which proof that the ATTACKER can win auction with minimum fund (1 wei in the PoC).

Running 1 test for test/AuctionDemoTest2.t.sol:AuctionDemoTest2
[PASS] testAttackerMultipleActiveBid() (gas: 1279087)
Logs:
  9999999999999999999999

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.57ms

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

Tools Used

Manual code review, Forge

Recommended Mitigation Steps

1. Only allow a single active bid from each user for every single auction. If the participate re-bid with higher price, old bid should be cancelled and updated to latest bid.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Invariant the owner of the token will receive the funds broken, implementation is sending the fund to owner() which will be the address that deployed AuctionDemo.sol. The fund will not be at direct risk, since the address belongs to the protocol will be receiving the fund.

Proof of Concept

• Run forge init in project root

• setup deployer script and test file: Gist link: link

Filepath: 
- script/DeployProtocol2.s.sol
- test/AuctionDemoTest2.t.sol

• Run the test:

forge test --mt testAuctionTokenOwner -vv

• Which will yields the following output: Assertion will pass, which proof the fund is sent to owner() instead of token owner

Running 1 test for test/AuctionDemoTest2.t.sol:AuctionDemoTest2
[PASS] testAuctionTokenOwner() (gas: 1205318)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.41ms

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

Tools Used

Manual Review, Forge

Recommended Mitigation Steps

Replace owner() with ownerOfToken

File: AuctionDemo.sol
- 168:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
- 169:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);

+ 168:                 (bool success, ) = payable(ownerOfToken).call{value: highestBid}("");
+ 169:                 emit ClaimAuction(ownerOfToken, _tokenid, success, highestBid);

Assessed type

Context

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