NextGen - tpiliposian's results

Lines of code

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

Vulnerability details

Impact

The impact of this finding is that the AuctionDemo.sol contract could be susceptible to bid manipulation, potentially allowing a bidder to win an auction with a lower bid than initially placed. This issue could undermine the fairness and integrity of the auction system.

The initial lower bidder could win the auction with an exceptionally lower bid, which may not represent the true value of the item being auctioned. This could lead to unfair outcomes and manipulation of the auction results.

Proof of Concept

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

Here is the vulnerable code segment in the AuctionDemo contract:

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Attack Scenario:

1. An attacker initially places a lower bid.
2. An attacker then places a very high bid, so that nobody will agree to pay more, and effectively discourage others.
3. Other potential bidders are discouraged by the high bid.
4. An attacker cancels their bid before the auction ends, and wins with the initially placed lower bid.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider implementing one or more of the following mitigation steps:

1. Implement a bid lock-in period during which bidders cannot cancel their bids. This will prevent last-minute bid cancellations and potential manipulation.
2. Enforce a minimum bid increment, which requires each new bid to be a certain percentage higher than the previous one. This discourages artificially low initial bids.
3. Introduce a fee for canceling bids, especially if the cancellation occurs after a certain point in the auction. This discourages malicious behavior and ensures commitment from bidders.

Assessed type

Other

1. Wrong random number generator

Description

The XRandoms.sol contract is used to generate random word from a given 100-length words array. But the function getWord() returns this:

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

        if (id==0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1];
        }

This function is called from randomWord() function which can generate a random number from 0 to 99. This means that in case a random number is 0 or 1, it will take the first word from the array, and there is no chance of having a last word from the array.

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

Remediation

Delete else block from the getWord() function, or generate a random number from 0 to 100:

--        if (id==0) {
            return wordsList[id];
--        } else {
--            return wordsList[id - 1];
--        }

-- uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 100;
++ uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 101;

2. Missing boundaries on returnIndex function

Description

In the XRandoms.sol contract the function returnIndex() is used to return an index for a given parameter id but there are no checks if the id is not greater than 99 or 100 (depending on the fix to the previous Wrong random number generator issue).

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

    function returnIndex(uint256 id) public view returns (string memory) {
        return getWord(id);
    }

Remediation

Add missing checks for the id parameter, as written in the descriptions.

3. Missing zero address checks on setter functions

Description

In the RandomizerRNG.sol and RandomizerVRF.sol contracts, the updateCoreContract function is used to update the _gencore parameter. However, this function does not include a check to ensure that the provided _gencore address is not the zero address (address(0)). Allowing the zero address as a valid parameter can lead to unexpected behavior and potential vulnerabilities.

    function updateCoreContract(address _gencore) public FunctionAdminRequired(this.updateCoreContract.selector) { 
        gencore = _gencore;
        gencoreContract = INextGenCore(_gencore);
    }

In the RandomizerNXT.sol there are missing checks in the following functions:

    function updateRandomsContract(address _randoms) public FunctionAdminRequired(this.updateRandomsContract.selector) {
        randoms = IXRandoms(_randoms);
    }

    function updateAdminsContract(address _admin) public FunctionAdminRequired(this.updateAdminsContract.selector) {
        adminsContract = INextGenAdmins(_admin);
    }

    function updateCoreContract(address _gencore) public FunctionAdminRequired(this.updateCoreContract.selector) { 
        gencore = _gencore;
        gencoreContract = INextGenCore(_gencore);
    }

Remediation

A zero address check should be added at the beginning of the given functions.

4. Missing boundaries on RNG cost

Description

In the RandomizerRNG.sol contract, the updateRNGCost function is used to update the ethRequired parameter without any bounds or limits. This means that anyone with the appropriate permissions can set the ethRequired value to any arbitrary number, which could potentially lead to unintended behavior or economic vulnerabilities.

    function updateRNGCost(uint256 _ethRequired) public FunctionAdminRequired(this.updateRNGCost.selector) {
        ethRequired = _ethRequired;
    }

Remediation

Add appropriate limits or constraints on the values that can be set for the ethRequired parameter within the updateRNGCost function. These limits should be defined based on the requirements and constraints of the contract.

5. Lack of Signature Update Validation in artistSignature Function

Description

The artistSignature function in the NextGenCore.sol contract allows an artist to submit their signature for a collection. However, there is no validation mechanism to prevent or correct mistakes if the _signature input is set to an undesired value, such as 0. This could lead to incorrect or unintended signatures for a collection, and once a signature is set, there is no way to update it.

    function artistSignature(uint256 _collectionID, string memory _signature) public {
        require(msg.sender == collectionAdditionalData[_collectionID].collectionArtistAddress, "Only artist");
        require(artistSigned[_collectionID] == false, "Already Signed");
        artistsSignatures[_collectionID] = _signature;
        artistSigned[_collectionID] = true;
    }

Remediation

Add an empty _signature check in the given function.