Skip to main content
Dedaub

SimplyVC

Smart Contract Security Assessment

January 16, 2023

Simply_Staking

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of the TulipFarm protocol. The codebase was delivered to us in the form of .zip files.

This audit report covers the contracts of the TulipFarm protocol, along with a number of RNG-related contracts which interact with TulipFarm. In particular, the RNG contracts allow randomness to be extracted from future blockhashes, and from the Chainlink VRF V1 and V2 APIs.

Two auditors worked on the codebase for 7 days on the following contracts:

TulipFarm

contracts/
  • MultipleWinners.sol
  • PeriodicPrizeStrategy.sol
  • Timelock.sol
  • TulipArt.sol
  • interfaces/
    • IERC2981.sol
    • ITulipArt.sol
    • ITulipMarket.sol
    • ITulipToken.sol
    • RNGInterface.sol
  • libraries/
    • ERC2981.sol
    • FixedPoint.sol
    • OpenZeppelinSafeMath_V3_3_0.sol
    • SortitionSumTreeFactory.sol (*)
    • UniformRandomNumber.sol
  • marketplace/
    • TulipMarket.sol
  • tokens/
    • LandToken.sol
    • TulipToken.sol

RNG

contracts/
  • RNGBlockhash.sol
  • RNGChainlink.sol
  • RNGChainlinkV2.sol
  • RNGChainlinkV2Interface.sol
  • RNGInterface.sol

(*) The SortitionSumTreeFactory.sol contract is considered trusted code by SimplyVC. The contract was reviewed to understand its interoperability with the rest of the system. No security audit was performed on this contract.


SETTING & CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behaviour. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

C1

Highest bidder is able to drain the funds of the marketplace due to the lack of reentrancy guards

C1CRITICAL

Highest bidder is able to drain the funds of the marketplace due to the lack of reentrancy guards
resolved

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;
}

C2

Highest bidder can get the NFT and his entire bid back due to the lack of cross-function reentrancy guards

C2CRITICAL

Highest bidder can get the NFT and his entire bid back due to the lack of cross-function reentrancy guards
resolved

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.



HIGH SEVERITY

H1

TulipMarket highest bidder can have its place taken by any user at no cost

H1HIGH

TulipMarket highest bidder can have its place taken by any user at no cost
resolved

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);
}

H2

Burning of an NFT can result in the highest bidder’s LAND tokens becoming trapped in TulipMarket contract

H2HIGH

Burning of an NFT can result in the highest bidder’s LAND tokens becoming trapped in TulipMarket contract
resolved

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.



MEDIUM SEVERITY

M1

TulipMarket auction can be disrupted by an attacker

M1MEDIUM

TulipMarket auction can be disrupted by an attacker
resolved

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.

M2

Listing an NFT disables bidding on the NFT

M2MEDIUM

Listing an NFT disables bidding on the NFT
resolved

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.



LOW SEVERITY

[No low severity issues]


CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

N1

Infinite minting of LAND token

N1CENTRALIZATION

Infinite minting of LAND token
dismissed

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.



OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Redundant function calls in Timelock contract

A1ADVISORY

Redundant function calls in Timelock contract
resolved

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

A2

Redundant function calls in PeriodicPrizeStrategy contract

A2ADVISORY

Redundant function calls in PeriodicPrizeStrategy contract
resolved

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.

A3

Double initialisation in PeriodicPrizeStrategy contract

A3ADVISORY

Double initialisation in PeriodicPrizeStrategy contract
resolved

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;
...
}

A4

RNG Request Timeout is hardcoded in the constructor of the PeriodicPrizeStrategy contract

A4ADVISORY

RNG Request Timeout is hardcoded in the constructor of the PeriodicPrizeStrategy contract
resolved

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);

...
}

A5

Redundant storage of RoundInfo objects in TulipArt contract

A5ADVISORY

Redundant storage of RoundInfo objects in TulipArt contract
resolved

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;
}

A6

TulipArt constructor does not initialise the lottery contract

A6ADVISORY

TulipArt constructor does not initialise the lottery contract
info

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.

A7

Magic number used in TulipArt’s upgradeLottery() function cleanup code

A7ADVISORY

Magic number used in TulipArt’s upgradeLottery() function cleanup code
resolved

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;
}

A8

RNGChainlinkV2 contract truncates block.number value

A8ADVISORY

RNGChainlinkV2 contract truncates block.number value
dismissed

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);
}

A9

Unused libraries

A9ADVISORY

Unused libraries
resolved

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

A10

Blockhash as source of randomness

A10ADVISORY

Blockhash as source of randomness
dismissed

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).

A11

Compiler bugs

A11ADVISORY

Compiler bugs
info

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.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub