SimplyVC

In the TulipMarket contract, an NFT owner can accept a bid for his token by calling acceptBidForTulip(). However, this function lacks reentrancy guards and allows a malicious receiver/bidder to reenter the function multiple times and drain all the funds from the marketplace.

More specifically, upon transferring the NFT to the highest bidder the ERC721::safeTransferFrom() will run the ERC721::_checkOnERC721Received() callback which delegates the execution to the receiver.

He can then reenter in the acceptBidForTulip() function which will call the _transferFeesAndFunds() function and get another portion of the funds transferring the NFT back to him. This can be continued until all funds are drained.

TulipMarket::acceptBidForTulip()

function acceptBidForTulip(
    uint256 _tokenId,
    uint256 _minPrice
) external override {
    ...
    // Send amounts + get royality fee
    _transferFeesAndFunds(_tokenId, address(this),
        msg.sender, highestBid.buyPrice);
    // Dedaub: Receiver can reenter at this point multiple times and make
               the above function be executed more than expected allowing
               him to drain all the funds of the marketplace
    IERC721(tulipNFTToken).safeTransferFrom(
        msg.sender, highestBid.buyer, _tokenId);

    emit PurchasedTulip(highestBid.buyer, _tokenId,
        msg.sender, _minPrice);

    _clearListing(_tokenId);

    highestBid.buyer = address(0);
    highestBid.buyPrice = 0;
}

Following on the C1 issue above, even not allowing the receiver to reenter in the acceptBidForTulip() function of the TulipMarket contract by placing a single function reentrancy guard, there is another opportunity for the buyer to profitably exploit the protocol.

Upon receiving the NFT after the seller accepts the highest bid, due to the lack of reentrancy guards and the fact that the updates of the state variables happen after the NFT transfer is completed, the buyer/attacker can reenter the contract by calling the cancelBidOnTulip() function.

TulipMarket::acceptBidForTulip()

function acceptBidForTulip(
    uint256 _tokenId,
    uint256 _minPrice
) external override {
    ...
    // Send amounts + get royality fee
    _transferFeesAndFunds(_tokenId, address(this),
        msg.sender, highestBid.buyPrice);
    // Dedaub: Receiver can reenter at this point to the contract by
    //         calling cancelBidOnTulip() for taking advantage of the
    //         fact that the highestBid data hasn't been updated yet
    IERC721(tulipNFTToken).safeTransferFrom(
        msg.sender, highestBid.buyer, _tokenId);

    emit PurchasedTulip(highestBid.buyer, _tokenId,
        msg.sender, _minPrice);
    // Dedaub: Here happens the data update and cleansing, after the NFT
    //         transfer has been completed
    _clearListing(_tokenId);

    highestBid.buyer = address(0);
    highestBid.buyPrice = 0;
}

Then, since the highestBid’s data hasn’t been cleared, he can get his LAND tokens back, draining some of the contract’s funds coming from bids for other NFTs, receiving at the same time his NFT normally. Hence, he ends up having the NFT and his entire bid amount back.

TulipMarket::cancelBidOnTulip()

function cancelBidOnTulip(
    uint256 _tokenId
) external override requireTulipExists(_tokenId) {
    Bid storage highestBid = tulipBiddings[_tokenId];
    // Dedaub: Reentering this function will not stop at this check
    //         because the highestBid data hasn't been updated yet
    require(highestBid.buyer == msg.sender,
        "TulipMarket/message-sender-does-not-bidder");
    // Dedaub: Buyer gets his tokens back draining the rest of the
    //         contract's fund coming from bids for other NFTs
    IERC20(landToken).transfer(highestBid.buyer, highestBid.buyPrice);

    emit BidCancelled(highestBid.buyer, _tokenId, highestBid.buyPrice);

    highestBid.buyer = address(0);
    highestBid.buyPrice = 0;
}

As a result, we highly recommend adding cross-function reentrancy guards (i.e. adding the nonReentrant guard to all function of the contract so that no function can be reentered by any other).

Moreover, it is generally safer to have any storage updates done, if possible, before any external calls, which can involve untrusted parties and could potentially lead to anauthorized use of the protocol. For example, in acceptBidForTulip() the NFT transfer can be moved at the bottom of the function after the cleansing of the highestBid data for ensuring that any important storage updates will have been completed and no old data could potentially be reused as part of an attack.

In the TulipMarket contract, the function bidOnTulip() should ensure the _amount parameter is strictly greater than highestBid.buyPrice. Otherwise, an attacker could bid the same amount as the highestBid and override another user’s highest bid at no cost.

TulipMarket::bidOnTulip()

function bidOnTulip(
    uint256 _amount,
    uint256 _tokenId
) external override requireTulipExists(_tokenId) {
    Bid storage highestBid = tulipBiddings[_tokenId];

    require(_amount > 0, "TulipMarket/cannot-bid-0-tokens-for-tulip");

    // Dedaub: An attacker can bid the same amount as the highest bid and
               overwrite the previous highest bidder at no cost
    require(_amount >= highestBid.buyPrice,
        "TulipMarket/bid-is-not-higher-than-current-bid");
    IERC20(landToken).
        safeTransferFrom(msg.sender, address(this), _amount);
    if (highestBid.buyPrice > 0) {
        IERC20(landToken).transfer(highestBid.buyer, highestBid.buyPrice);
    }

    highestBid.buyer = msg.sender;
    highestBid.buyPrice = _amount;

    emit BidOnTulipCreated(msg.sender, _tokenId, _amount);
}

In the TulipMarket contract, the function cancelBidOnTulip() has the modifier requireTulipExists(tokenId), which effectively checks that the owner of the NFT in question is not the zero address.

Tulipmarket::cancelBidOnTulip()

function cancelBidOnTulip(uint256 _tokenId
) external override requireTulipExists(_tokenId) {
    ...
}

TulipMarket::requireTulipExists

modifier requireTulipExists(uint256 _tokenId) {
    // Dedaub: Reverts when the given _tokenId doesn't belong to anyone
    IERC721(tulipNFTToken).ownerOf(_tokenId);
    _;
}

Consider the situation where a user calls buyTulip(), making the highest bid on the NFT redundant. Then, before the old highest bidder can call cancelBidOnTulip() to obtain his funds back, the new owner of the NFT burns the NFT by calling the burn() function of the ERC721 contract, which sets the ownership of the NFT to the zero address.

This would result in the highest bidder not being able to retrieve his LAND tokens due to an unrelated action carried out by the new owner.

Funds can also be trapped if the seller does not list the NFT (thus maintaining control of it during the auction) and then burns it after the bid has taken place.

In the TulipMarket contract, the bidOnTulip() function overrides the previous bid if a greater amount is proposed. No record of the previous bid is kept. This means that an attacker can replace the highest bid with a marginally higher bid, and then subsequently call cancelBidOnTulip() to cancel the same bid. As a result, there will be no resulting bid on the NFT.

SimplyVC has indicated that a user can offer both auction and buy functionality at the same time to potential bidders and buyers.

Now, in order to enable buy functionality, the NFT first has to be listed using the listTulip() function. This function transfers the NFT from the user to the TulipMarket contract, changing its ownership to the contract.

TulipMarket::listTulip():75

function listTulip(
    uint256 _salePrice,
    uint256 _tokenId,
    address _onlySellTo
) external override {
    require(
        IERC721(tulipNFTToken).ownerOf(_tokenId) == msg.sender,
        "TulipMarket/not-owner-of-tulip"
    );

    // Dedaub: The NFT is transferred to the contract
    IERC721(tulipNFTToken).safeTransferFrom(
        msg.sender,
        address(this),
        _tokenId
    );
    ...
}

Now, suppose that the seller subsequently decides to accept a bid by calling acceptBidForTulip(). This function checks that the NFT is still owned by the seller.

TulipMarket::acceptBidForTulip():173

function acceptBidForTulip(
    uint256 _tokenId,
    uint256 _minPrice
) external override {

    Bid storage highestBid = tulipBiddings[_tokenId];

    // Dedaub: This requirement disallows a seller to accept a bid when he
               has also enlisted the NFT in the marketplace
    require(
        IERC721(tulipNFTToken).ownerOf(_tokenId) == msg.sender,
        "TulipMarket/message-sender-not-owner-of-tulip"
    );
    require(
        highestBid.buyPrice >= _minPrice,
        "TulipMarket/min-price-is-lower-than-bid-price"
    );
    ...
}

But at this stage, the owner of the NFT is no longer the seller but the TulipMarket contract. Since this call will fail, this means that bidding functionality is effectively disabled when a user also lists an NFT for potential buyers.

The LandToken contract allows the owner to mint an unbounded amount of tokens which can dilute the value of the LAND token.

LandToken::mint()

function mint(address _beneficiary, uint256 _amount) public onlyOwner {
    _mint(_beneficiary, _amount);
}

This has been disclosed in the README.md file of the project by the SimplyVC team.

In the Timelock contract, the function getBlockTimestamp() is a wrapper for block.timestamp and can be refactored away.

In the PeriodicPrizeStrategy contract, the function _currentBlock() is a wrapper for block.number and the function _currentTime() is a wrapper for block.timestamp and can be refactored away.

In the PeriodicPrizeStrategy contract, the function initialize() initialises prizePeriodBlock twice, first through a call to _setPrizePeriodBlocks() and then through the statement prizePeriodBlocks = _prizePeriodBlocks.

PeriodicPrizeStrategy::initialize()

function initialize(
    uint256 _prizePeriodStart,
    uint256 _prizePeriodBlocks,
    ITulipArt _tulipArt,
    RNGInterface _rng
) public initializer {
    ...
    tulipArt = _tulipArt;
    rng = _rng;
    // Dedaub: prizePeriodBlocks gets initialized here
    _setPrizePeriodBlocks(_prizePeriodBlocks);

    __Ownable_init();

    // Dedaub: Redundant assignment
    prizePeriodBlocks = _prizePeriodBlocks;
    prizePeriodStartedAt = _prizePeriodStart;
    ...
}

In the PeriodicPrizeStrategy contract, the constructor sets the value of the rngRequestTimeout variable to 1800, when this could have been provided as a parameter to the constructor for additional configurability.

PeriodicPrizeStrategy::initialize()

function initialize(
    uint256 _prizePeriodStart,
    uint256 _prizePeriodBlocks,
    ITulipArt _tulipArt,
    RNGInterface _rng
) public initializer {
    ...

    // Dedaub: Hardcoded RNG Request Timeout
    _setRngRequestTimeout(1800);

    ...
}

In the TulipArt contract, the function _createNextRound() pushes a new RoundInfo object on the roundInfo array during each round. This is wasteful since only the last RoundInfo item of the array is ever accessed by the contract. This occurs because every access to the array is carried out through the _currentRound() function, which returns the index of the last round.

TulipArt::_createNextRound()

function _createNextRound(uint256 _id) internal {
    roundInfo.push(
        RoundInfo({roundId: _id,
                   roundState: RoundState.OPEN
    }));
    emit RoundUpdated(_id, RoundState.OPEN);
}

In addition, the totalRounds() function can be refactored to work with a counter and does not need to use RoundInfo.length.

TulipArt::totalRounds()

function totalRounds() public view returns (uint256) {
    return roundInfo.length;
}

The constructor of the TulipArt contract does not set the lotteryContract variable. Hence, setting the lottery contract for the first time has to go through the proposeLottery() and upgradeLottery() functions, which means that there will be a delay before the lotteryContract can be set the first time.

The upgradeLottery() function of the TulipArt contract cleans up the lotteryCandidate variable by setting lotteryCandidate.proposedTime = 5000000000. It also sets lotteryCandidate.implementation to address(0).

However, lotteryCandidate.proposedTime can simply be set to 0 as well since upgradeLottery() cannot be called again when lotteryCandidate.implementation == address(0).

TulipArt::upgradeLottery()

function upgradeLottery() public onlyOwner {
    require(
        roundInfo[_currentRound()].roundState == RoundState.OPEN,
        "TulipArt/round-not-open"
    );

    // Dedaub: This prevents the function from being called again without
               calling proposeLottery() first
    require(
        lotteryCandidate.implementation != address(0),
        "TulipArt/there-is-no-candidate"
    );

    if (lotteryContract != address(0)) {
        require(
            lotteryCandidate.proposedTime.add(approvalDelay) <
                block.timestamp,
            "TulipArt/delay-has-not-passed"
        );
    }

    emit UpgradeLottery(lotteryCandidate.implementation);

    lotteryContract = lotteryCandidate.implementation;
    lotteryCandidate.implementation = address(0);

    // Dedaub: Redundant assignment
    lotteryCandidate.proposedTime = 5000000000;
}

In the requestRandomNumber() function of the RNGChainlinkV2 contract, the value of block.number is truncated from uint256 to uint32. This value is stored by the contract and left unused, however, please note that such a truncation may introduce problems in the future.

RNGChainlinkV2::requestRandomNumber()

function requestRandomNumber()
    external override onlyManager
    returns (uint32 requestId, uint32 lockBlock)
{
    ...
    requestId = _requestCounter;
    chainlinkRequestIds[_vrfRequestId] = _requestCounter;
    // Dedaub: block.number truncation from uint256 to uint32
    lockBlock = uint32(block.number);
    requestLockBlock[_requestCounter] = lockBlock;

    emit RandomNumberRequested(_requestCounter, msg.sender);
  }

The FixedPoint.sol and OpenZeppelinDafeMath_V3_3_0.sol libraries are unused by the project.

The project appears to be able to use three sources of randomness. This can be provided by future blockhashes (RNGBlockhash.sol contract), and the Chainlink VRF V1 and V2 APIs.

Please be aware that under PoS the blockhash of a future block is completely manipulable by Ethereum validators, who can try and generate multiple blocks until one with a desired blockhash appears. This can result in a malicious validator choosing the winner of the NFT lottery when he is chosen by the protocol to create a block.

The recommended approach under PoS is to use a future block.difficulty instead. This instruction returns Ethereum’s PREVRANDAO value for that block (see EIP-4399).

The code of the TulipFarm contracts is compiled with Solidity 0.6.12 and the code for the RNG contracts is compiled with Solidity 0.8.6. Both versions 0.6.12 and 0.8.6, in particular, have some known bugs, which we do not believe affect the correctness of the contracts.