SimplyVC TulipFarm
Smart Contract Security Assessment
21.01.2021
SUMMARY
ABSTRACT
Dedaub was commissioned to perform an audit on the TulipFarm smart contracts. The codebase was delivered to us in the form of a .zip file on January 17th.
The primary contracts audited are listed below:
RNGChainlink.sol
MultipleWinners.sol
tokens/TulipToken.sol
tokens/LandToken.sol
PeriodicPrizeStrategy.sol
TulipArt.sol
The audit also examined any other functionality highly related to the included contracts. More specifically, the Chainlinks VRF contracts which interact with TulipFarm.
SETTING & CAVEATS
The audited codebase is of a rather small size, slightly larger than ~1KLoC. The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.
ARCHITECTURE & HIGH-LEVEL RECOMMENDATIONS
The TulipFarm contracts form a lottery game. Users are invited to stake LAND tokens in the staking contract TulipArt, in order to participate in the game. In each round, a number of winners receive a newly minted NFT token - a TulipToken. The number of winners is a configurable parameter set by the owner of the lottery contract (MultipleWinners-PeriodicPrizeStrategy). The winners of each round are randomly selected based on a random number requested by an external (on-chain) source: a VRF (Verifiable Random Function) random number generator contract developed by Chainlink. The addresses of the stakeholders are stored in a specialised tree data structure, which allows for the possibility of winning to be proportional to the size of the corresponding stake. The interaction with the VRF generator is handled by the RNGChainlink contract. Contracts MultipleWinners, PeriodicStrategy and RNGChainlink are adapted forks of Pooltogether’s corresponding contracts.
Some centralization issues exist: in order to be able to take actions upon upgrades or issues of the external contracts interacting with TulipFarm, the Owner is privileged to set sensitive parameters of the protocol at any time. Such parameters would be the address of the random generator contract, the timeout value based on which a request to the random generator expires, and the controller’s address in TulipToken contract which is responsible to set the winners of the NFTs. Similarly, the owner is able to upgrade the Lottery contract address in the TulipArt staking contract. However, a timelock-similar flow is followed in this case, which alleviates the severity of the centralization issue since the users can monitor the proposed changes and take actions on time, if need be.
In general, the codebase is well written with plenty of helpful comments. No critical, high or medium severity issues were found.
VULNERABILITIES & FUNCTIONAL ISSUES
This section details issues that affect 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:
- 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.
- 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”, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
[No critical severity issues]
HIGH SEVERITY
[No high severity issues]
MEDIUM SEVERITY
[No medium severity issues]
LOW SEVERITY
In the contract PeriodPrizeStrategy the variables PrizePeriodStartedAt and PrizePeriodSeconds carry values which are denominated in seconds. These two variables are supposed to define the active period of time during which For each round that is to be completed by distributing the prizes, PrizePeriodStartedAt is set as if prize periods can start only in specific time slots relative to the prizePeriodSeconds:
function completeAward() external requireCanCompleteAward {
uint256 randomNumber = rng.randomNumber(rngRequest.id);
delete rngRequest;
_distribute(randomNumber);
// Dedaub: possible inconsistency
prizePeriodStartedAt = _calculateNextPrizePeriodStartTime(_currentTime());
...
}
function _calculateNextPrizePeriodStartTime(uint256 currentTime)
internal
view
returns (uint256)
{
uint256 elapsedPeriods = currentTime.sub(prizePeriodStartedAt).div(
prizePeriodSeconds
);
return prizePeriodStartedAt.add(elapsedPeriods.mul(prizePeriodSeconds));
}
However, it doesn't seem trivial to reason about the long term consistency of the consequent prize periods: on the one hand there is the "no clock drifts" of PrizePeriodStartedAt while, on the other hand, accumulated delay may occur due to delayed calls of completeAward.
The possibility of an overlap between the prize periods seems rather low, since a number of actions need to be conducted between them - e.g. request randomness and wait for it. However, since the protocol doesn’t seem to need the current complexity, we suggest changing the time unit from seconds to number-of-blocks. This would result in a much more simple design and codebase, making it easier to reason about its security but also to maintain.
OTHER/ ADVISORY ISSUES
This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.
Contract PeriodicPrizeStrategy is defined as follows:
abstract contract PeriodicPrizeStrategy is Initializable, OwnableUpgradeable {
Direct inheritance from Initializable can be omitted, since it is already indirectly inherited by OwnableUpgradeable:
abstract contract OwnableUpgradeable is Initializable, ContextUpgradeable {
In RNGChainlink.sol functions _requestRandomness() and fulfillRandomness() use different naming for a request’s internal id and the id given to it by chainlink’s VRF contract:
function _requestRandomness(uint256 seed) internal returns (uint32 requestId)
{
// Get next request ID
// Dedaub: "requestId" is the internal id
requestId = _getNextRequestId();
// Complete request
bytes32 vrfRequestId = requestRandomness(keyHash, fee, seed);
chainlinkRequestIds[vrfRequestId] = requestId;
emit VRFRequested(requestId, vrfRequestId);
}
// Dedaub: would be more clear to keep the naming of internalRequestId/requestId and vrfRequestId along the contract for clarity
function fulfillRandomness(bytes32 requestId, uint256 randomness)internal override
{
// Dedaub: "requestId" is the id assigned by the VRF contract
uint32 internalRequestId = chainlinkRequestIds[requestId];
// Store random value
randomNumbers[internalRequestId] = randomness;
emit RandomNumberCompleted(internalRequestId, randomness);
}
We suggest adopting the names internalRequestId and vrfRequestId throughout the contract for clarity.
In TulipArt.sol variables
address public landToken;
address public tulipNFTToken;
are set during the contract’s construction and can never be altered.
We suggest defining them as immutables for clarity and slight gas optimization.
In TulipToken.sol a call to changeControllerRole that doesn’t alter the current role of a controller will be successfully executed, causing unnecessary gas costs but also emitting an event that could be confusing. We suggest adding a requirement checking that the new role to be set differs from the current one.
Some of the TulipFarm contracts use the floating pragma pragma solidity ^0.6.12;
Floating pragma allows the contracts to be compiled with any 0.6.x version greater than 0.6.12. Since this is the last version of 0.6.x the floating pragma can be simply set to pragma solidity 0.6.12. Also some contracts have floating pragma: pragma solidity >=0.6.0 <0.7.0;. Although the differences between these versions are small, floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment.
The contracts were compiled with several versions of the Solidity compiler v0.6 which, at the time of writing, have some known bugs. We inspected the bugs listed for these versions and concluded that the subject code is unaffected.
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.