CrowdVilla Smart Contract Audit

The ReiDAO team engaged New Alchemy™ to audit the smart contracts for the token sale of the CrowdVilla Token. We focused on identifying flaws in the code or in the design of the contracts that would have a security impact. Here are the results —

The audited code is located in the REIDAO/crowdvilla-token-sale repository and the version audited in this report is commit hash 306147bcea3f9e2fc1435a1cd0a19e295d65c8e5.

Update: The REIDAO — Digitising Real Estate Ownership team has followed our recommendations and updated the CrowdVilla Token contract. New Alchemy has audited the new version, at commit hash d32623d0b2faf90fe0cbe912caf2f17c9ea08700, to ensure that the previously identified issues were remedied and no new issues were introduced.

The audit identified no critical issues, but did locate one issue with moderate impact and an additional six with minor impact. The most severe issue allows locked tokens to be transferred; the others deal with contract methods that are unnecessarily expensive for callers in terms of gas, a potential for contracts to become unowned, unchecked arithmetic, and a lack of protections against short-address attacks.

General Discussion

There are fourteen token contracts in the repository with a complicated inheritance structure:

They appear to have been designed to keep different functionality in different contracts. However, this separation makes it more difficult to track what method is being called, and as in the case of the canTransfer flaw described below, this can lead to bugs.

The contract hierarchy could be simplified as follows:

•REIDAOMintableBurnableToken is never inherited or instantiated. Consequently, it can be removed without affecting anything.

•MintableToken, BurnableToken, and REIDAOMintableLockableToken (and once REIDAOMintableBurnableToken is removed, REIDAOBurnableToken as well) are never instantiated and are only inherited by a single contract. They can be folded into their subcontracts. With these changes, there would only be 10 token contracts remaining (including three leaf contracts and five derived from the open-source OpenZeppelin project) with a much simpler hierarchy, reducing the chance of logic bugs. UPDATE: Reitech Pte Ltd. opted to preserve the current design in order to use further subclasses REIDAOMintableBurnableToken and REIDAOMintableLockableToken in future contracts.

A number of contracts currently require users to make unnecessary state changes. As changing state is one of the more expensive EVM opcodes, this can unfairly penalize users. REIDAO should take care to minimize the gas use of function calls that users are expected to make.

Moderate Issues

Locked CRV and CRP Tokens can be transferred

REIDAOMintableLockableToken creates a registry of tokens that are “locked” until a certain time. Until this time is reached, they should not be usable, though they are still owned by the original owner. This contract also creates a modifiercanTransferthat checks if a requested number of tokens are not locked. However, this contract inherits BasicToken and StandardToken but does not override BasicToken.transfer or StandardToken.transferFrom. Consequently, any user that owns locked tokens can still transfer them. In fact, nothing ever uses canTransfer.

To address this issue, this contract should override BasicToken.transfer or StandardToken.transferFrom in order to apply the canTransfer modifier to them.

Re-test results: This issue has been fixed as recommended.

Minor Issues

Contracts can become unowned

Unlike the common single-owner pattern from OpenZeppelin’s Ownable contract, REIDAO’s Owners contract (ownership/Owners.sol) allows multiple owners. Owners can add or remove other owners. Consequently, if a contract that inherits Owners has a single owner that then removes itself, that contract would then become unowned. In order to avoid this, owners should be prohibited from removing themselves.

Reitech Pte Ltd. engineers indicated that they intend there to always be at least two owners. This could be enforced in code as well.

Re-test results: Ownable now tracks the number of owners, prevents owners from being removed if that would result in fewer than two owners, and prevents owners from removing themselves.

However, a minor flaw remains in that removeOwner doesn’t check that the address being removed is actually an owner. Removing a non-owner will not affect the contract’s state but will result in the OwnerRemoved event being triggered, which could confuse some listeners.

Late contributors pay for unnecessary state changes

If some contributor calls the fallback function of CrowdvillaTokenSale (contracts/CrowdvillaTokenSale.sol line 105) in or after block 52800, but before an owner calls endTokenSale to set the state of the sale to End, then that contributor will have to change state to End and pay the associated gas cost. Changing state is one of the more expensive Solidity opcodes at 20,000 gwei, so this unfairly penalizes the contributor.

This can be improved upon by replacing lines 112–114 with require(block.number < saleEndBlock);; this way the contributor doesn’t need to pay for the state change or return transfer. Subsequent contributors calling into the default function before state is changed will also fail this check, rather than failing the state check at line 111. This results in a trivial gas penalty to all users who call the function after the sale ends rather than an expensive penalty for the first one and slightly smaller trivial penalties to all others.

Re-test results: This issue has been fixed as recommended.

High-value contributors pay for unnecessary state changes

The logic for updating stretch goals in CrowdvillaTokenSale(starting at line 144 of contracts/CrowdvillaTokenSale.sol) is unnecessarily complicated and unfair to any users that make extremely large contributions, since such a contributor may end up paying to change state more than once. Since this gas penalty is trivial in comparison to the amount of Ether that a contributor must contribute to experience the issue, this is a very minor concern. However, an improvement might be to add require(currentStretchGoal < stretchGoals.length) to the top of the block at line 112 and replace lines 144 through 151 with something akin to the following:

for (uint currGoal = currentStretchGoal;
    currGoal < stretchGoals.length && totalFund >=    stretchGoals[currGoal];
    currGoal++ {
}
if (currGoal != currentStretchGoal)
    currentStretchGoal = currGoal;
}

Early contributors in REI token sale pay for unnecessary state changes

The mechanism to prevent contributors from purchasing REI tokens before the sale begins could be improved to consume less gas. The early check in the function only takes into account a paused sale state and since the default value for state is interpreted as stage 1, the function will do work and end up refunding the contributor’s funds. The following check could be used to address this problem:require(stageAvailableTokens > 0)

Re-test results: This issue has been fixed as recommended.

Lack of short address attack protections

Some Ethereum clients may create malformed messages if a user is persuaded to call a method on a contract with an address that is not a full 32 bytes long. In such a “short-address attack”, an attacker generates an address whose last byte is 0x00, then sends the first 31 bytes of that address to a victim. When the victim makes a contract method call, it appends the 31-byte address to msg.datafollowed by a value. Since the high-order byte of the value is almost certainly 0x00, reading 32 bytes from the expected location of the address in msg.data will result in the correct address. However, the value is then left-shifted by one byte, effectlively multiplying it by 256 and potentially causing the victim to transfer a much larger number of tokens than intended. msg.data will be one byte shorter than expected, but due to how the EVM works, reads past its end will just return 0x00.

This attack effects methods that transfer tokens to destination addresses, where the method parameters include a destination address followed immediately by a value. In the REIDAO contracts, such methods include BasicToken.transfer , MintableToken.mint , StandardToken.transferFrom , StandardToken.approve , StandardToken.increaseApproval , StandardToken.decreaseApproval , and REIDAOMintableLockableToken.canTransfer.

While the root cause of this attack is buggy serializers and how the EVM works, it can be easily mitigated in contracts. When called externally, an affected method should verify that msg.data.length is at least the minimum length of the method’s expected arguments (for instance, msg.data.length for an external call to BasicToken.transfer should be at least 68: 4 for the hash, 32 for the address, and 32 for the value). This can be implemented in a modifier. External calls can be detected in the following ways:

• Compare the first four bytes of msg.data against the method hash. If they don’t match, then the call is internal and no short-address check is necessary.
• Avoid creating public methods that may be subject to short-address attacks; instead create only external methods that check for short addresses as described above. public methods can be simulated by having the external methods call private or internal methods that perform the actual operations and that do not need to check for short-address attacks.

Re-test results: This issue has been partially addressed. BasicToken.transfer, MintableToken.mint, StandardToken.transferFrom , StandardToken.approve , StandardToken.increaseApproval , and StandardToken.decreaseApproval now apply a modifier that verifies that msg.data.length is equal to the expected parameter size; since REIDAOMintableLockableToken.canTransfer does not modify state, it is not necessary that it checks for short addresses. Additionally, an internal version of MintableToken.mint was added that does not check msg.data.length. However, since the current implementation checks that msg.data.length is equal to the expected size, it may not accept calls from client software that pads msg.data; the recommended check is that msg.data.length is greater than or equal to the expected length. All of the listed functions remain public , so they may now fail if future code ever makes internal calls to them.

Unchecked arithmetic

While most of the CrowdvillaTokenSale contract (such as getPromisedREITokenAmount) uses SafeMath for arithmetic, getPromisedCRVTokenAmount and getREIDAODistributionTokenAmount use operators. For safety, all should use SafeMath; it is difficult to ensure that all possible overflow scenarios have been covered and much easier to use checked arithmetic.

Re-test results: This issue has been fixed as recommended.

Line by line comments

This section lists comments on design decisions and code quality made by New Alchemy during the review. They are not known to represent security flaws.

contracts/ownership/Owners.sol

Line 26: addOwner

When adding an owner, consider adding a check for the zero address. This can catch calls to the function where parameters may not have been initialized.

Re-test results: This issue has been fixed as recommended.

contracts/registries/CRPAllocationConfig.sol

Line 7

struct config is never used and should be removed.

Re-test results: This issue has been fixed as recommended.

Line 34: addConfig

REIDAO can use this contract to change the token allocation. However, there is no check that the allocation percentages add to 100; if they don’t, then CRPGeneration may not work as expected. Also, this contract allows additional allocation holders to be added with their own percentages, but without additional changes to CRPGeneration, it will not know about the new holders.

In order to make this contract more robust, it should simply store allocation percentages for the four specific holders. Any changes to the allocation should then be made to all four at once, with an assertion that the allocation adds to 100%, as in the code below. Since changes to the number of allocation holders already requires changes to both CRPAllocationConfig and CRPGeneration, such a change would not make that process more difficult.

contract CRPAllocationConfig is Owners(true) {
  uint public allocTokenHolder;
  uint public allocCrowdvillaNpo;
  uint public allocCrowdVillaOps;
  uint public allocReidao;
  function CRPAllocationConfig() public {
    setConfig(30, 50, 15, 5);
  }
  function setConfig(uint _allocTokenHolder, uint _allocCrowdvillaNpo, 
                     uint _allocCrowdvillaOps, uint _allocReidao) public ownerOnly {
    assert(100 == _allocTokenHolder + _allocCrowdvillaNpo + _allocCrowdvillaOps 
                  + _allocReidao);
    allocTokenHolder = _allocTokenHolder;
    allocCrowdvillaNpo = _allocCrowdvillaNpo;
    allocCrowdvillaOps = _allocCrowdvillaOps;
    allocReidao = _allocReidao;
  }
  // ...
}

(Note: New Alchemy has not tested this code.)

Also consider triggering an event when the configuration changes so that users can be notified.

Re-test results: This issue has been fixed as recommended.

contracts/registries/CRPGenerationConfig.sol

Line 31: addConfig

There is a comment missing for the _subseqPct function parameter. Additionally, the comment for the _subseqFreqparameter should be for the _subseqPct parameter.

Re-test results: This issue has been fixed as recommended.

Line 36: addConfig

The function allows adding a CRP configuration. There is a dependency between three of the values such that they combine to equal 100%. Consider adding a check that validates the percent of CRP tokens:

initPct + subseqPct * subseqFreq = 100

Also consider triggering an event when the configuration changes so that users can be notified.

Re-test results: This issue has been fixed as recommended.

contracts/tokens/MintableToken.sol

Line 57: startMinting

MintableToken allows minting to be stopped (finishMinting) and restarted (startMinting). Since this contract is inherited by REIToken, this allows an REIToken owner to resume minting tokens even after finishMinting has been called. This is presumably because there are several REI Token issuance periods. However, there is no guarantee that the fixed supply of 5,000,000 REI Tokens guaranteed by the REI Token whitepaper is not exceeded. This functionality could allow a rogue owner to resume minting tokens even after all documented issuances had completed.

This issue could be addressed by removing startMinting so that finishMinting is final, then not calling finishMinting until all issuances are complete. A somewhat more robust solution would be to call finishMintingafter each issuance, but either track the number of tokens issued and disallow further minting once the limit is reached or keep track of the current allocation phase and disallow further startMinting calls once all allocation phases are complete.

Re-test results: Reitech Pte Ltd. opted to not make such a change in order to leave this contract as generic as possible.

contracts/tokens/REIDAOBurnableToken.sol

Lines 21, 29: addHostedWallet and removeHostedWallet

The functions addHostedWallet and removeHostedWallet are public allowing anyone to flag any wallet as "hosted", subsequently allowing it to call burn. This is not a big deal since nothing ever directly instantiates this contract, and of its subcontracts, the only ones that ever do get instantiated are also subcontracts of REIDAOMintableBurnableLockableToken, which adds the modifier ownerOnly to these two functions. However, care should be taken that nothing else is added that derives from this contract without adding further restrictions. A more robust solution would be to apply the ownerOnly modifier in REIDAOBurnableToken.

Re-test results: Since REIDAOBurnableToken does not inherit Ownable, Reitech Pte Ltd. opted to not make any changes here, but noted that future subclasses may need to handle this issue.

contracts/tokens/REIDAOMintableLockableToken.sol

Line 45: transferableTokens

The check for zero lockedTokens is unnecessary because even if it was zero, the subsequent subtraction operation would still hold the same semantics. Zero subtracted from a value is the value itself.

Re-test results: This issue has been fixed as recommended.

Line 60: getLockedTokens

The check for zero numLocks is unnecessary because even if it was zero, its usage as a for loop terminator would still hold the same semantics. The loop would never be entered.

Re-test results: This issue has been fixed as recommended.

contracts/tokens/REIToken.sol

Line 32: disburseToREIDAO

The logic in this function is deeply nested and hard to follow. It could be much more clearly expressed as follows:

function disburseToREIDAO() public ownerOnly {
  if (!tokensReleased[0]) {
    //for WAVE 1 - immediate
    disburseWave(1, 200000 * 10**decimals);
  } else if (block.timestamp >= 1577836800 && !tokensReleased[1]) {
    //for WAVE 2 - after 01/01/2020 @ 12:00am (UTC)
    disburseWave(2, 200000 * 10**decimals);
  } else if (block.timestamp >= 1609459200 && !tokensReleased[2]) {
    //for WAVE 3 - after 01/01/2021 @ 12:00am (UTC)
    disburseWave(3, 200000 * 10**decimals);
  } else if (block.timestamp >= 1609459200 && !tokensReleased[3]) {
    //for WAVE 4 - after 01/01/2022 @ 12:00am (UTC)
    disburseWave(4, 150000 * 10**decimals);
  } else {
    require(false);
  }
}
function disburseWave(uint wave, uint amount) private ownerOnly {
  assert(1 <= wave && wave <= 4);
  mint(wallet, amount);
  tokensReleased[wave-1] = true;
  WaveReleased(wave, amount);
}

(Note: New Alchemy did not test this re-written code to ensure that it is actually equivalent to the REIDAO version.)

Re-test results: This issue has been fixed as recommended.

contracts/CRPGeneration.sol

Line 50

The parameter name in the function comment block is incorrect. Change from crvToken to crvToLock.

Re-test results: This issue has been fixed as recommended.

Lines 67, 70: generateCRP

The comments assume certain hard-coded values for initPct and subseqFreq, but they could be different because they come from writable contract state.

Re-test results: This issue has been fixed as recommended.

Line 72: generateCRP

Inefficient use of gas when calling the crpToken.mintAndLockTokens function in the for loop. The second argument is calculated to the same value each iteration of the loop.

Re-test results: This issue has been fixed as recommended.

contracts/CrowdvillaTokenSale.sol

Lines 26, 27, 28

It should not be necessary to store crvTokenAddr, crpTokenAddr, and reiTokenAddr, as they are not read or written outside of the constructor.

Re-test results: Reitech Pte Ltd. opted to retain these state variables for ease of public verification of the deployed contracts.

Line 28

This contract stores its own configuration and addresses for wallets and other contracts. By comparison, CRPGeneration (contracts/CRPGeneration.sol) uses AddressesEternalStorage to manage addresses, allowing the implementation of one contract to change without needing to update every other contract that references it. Since this contract is of limited duration, it's not likely that addresses will need to change while it is active, but it is a risk.

Re-test results: Reitech Pte Ltd. opted to retain the original design for simplicity.

Lines 12, 34, 35

uniqueContributors, contributorIndex, and reversedContributorIndex are write-only variables within this contract. Reitech Pte Ltd. reports that they are kept for statistical purposes.

Line 96: constructor

An assertion that 0 < _stretchGoal1 < _stretchGoal2 < stretchGoal3 would be worthwhile. If this is not true, then various things elsewhere in the contract will not work.

Re-test results: This issue has been fixed as recommended.

Line 131

It is worth commenting that the magic string is keccak256(uint256(0)).

Re-test results: This issue has been fixed as recommended.

Line 146

This code will set currentStretchGoal to stretchGoals.length when all stretch goals have been reached. Subsequently, getPromisedCRVTokenAmount, getPromisedREITokenAmount, getREIDAODistributionAmount, and updateContributorAddress will use stretchGoals.length as a mapping key into contributions and contributionsPerStretchGoal, even though nothing should ever be written to this key. This will charge users unnecessary gas. However, the calculations of CRV to be assigned appear correct: it will assign tokens with a 0% bonus to 0 tokens.

Re-test results: Reitech Pte Ltd. added checks that the contributor made contributions towards each specific stretch goal in getPromisedCRVTokenAmount, getPromisedREITokenAmount, and getREIDAODistributionAmount; such a check was already present in updateContributorAddress. This greatly reduces the number of operations performed on contributions and contributionsPerStretchGoal at key stretchGoals.length.

Line 218: getREIDAODistributionTokenAmount

The arithmetic for calculating the token distribution to REIDAO is obtuse. The whitepaper (https://crowdvilla.io/whitepaper.pdf) indicates that 80% of the CRV issued during the sale goes to contributors and 20% goes to REIDAO. In the code, the calculation for CRV issued to users in getPromisedCRVTokenAmount does not include any deduction or withholding. Then getREIDAODistributionTokenAmount calculates REIDAO's share by adding up the total CRV due to all users then calculating 25% of that. This is mathematically correct: 20% is 25% of 80%. However, it makes this calculation using a 20% figure, resulting in unnecessarily convoluted arithmetic at lines 218 and 219. Perhaps a clearer way could be found to express this? Even a comment would help.

Re-test results: Reitech Pte Ltd. added comments clarifying the calculations somewhat.

Line 293: updateContributorAddress

Is it really appropriate to trigger the Contribute event here? No new contribution was made; the change was to the source of some existing contributions. Should there be a different event to indicate this?

Re-test results: Reitech Pte Ltd. opted to leave this code as it is based on their intended use of the Contributeevent.

contracts/REITokenSale.sol

Lines 37–39: constructor

The comments for constructor parameters _stage1MinTokens, _stage2MinTokens, and _stage3MinTokens indicate the value should NOT include decimals, but the code uses it assuming decimals are included.

Re-test results: Reitech Pte Ltd. added code to include decimals when these parameters are used.

Line 41: constructor

The wrong stage number is shown for the comment block for the _stage2TokenPrice constructor parameter. It shows stage 3, but it should be stage 2.

* @param _stage2TokenPrice uint the price per 1 REI in wei, in stage 3

Re-test results: This issue has been fixed as recommended.

Line 105: fallback function

The contract “state” is changed to Pause by setting the state variable directly, but the function pauseTokenSaleshould instead be called in order to keep other state variables in sync.

Re-test results: Reitech Pte Ltd. replaced this check with a requirement at the top of the function that stageAvailableTokens is non-zero.

Line 133: startTokenSale

The function could put the contract in an unexpected state if called with out of order stages. Consider adding checks such that the stages are entered in an increasing order one after the other.

Re-test results: This issue has been fixed as recommended.

contracts/ReferrerAcct.sol

Line 7: constructor

The code state variable is never used and should be removed.

Re-test results: Reitech Pte Ltd. intend code to be a write-only variable within this contract.

Line 18: constructor

Remove the superfluous tag @constructor in the function comment block. It is not a valid tag according to the Ethereum wiki.

Re-test results: This issue has been fixed as recommended.

migrations/2_deploy_contracts.js

Line 63

The wallet address stored in variable reidaoWallet on line 11 is used as a string literal, but the JavaScript variable should instead be used to keep the two locations in sync.

Re-test results: This issue has been fixed as recommended.

Disclaimer — The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug-free status. The audit documentation is for discussion purposes only.

New Alchemy is a strategy and technology advisory group specializing in tokenization. One of the only companies to offer a full spectrum of guidance from tactical technical execution to high-level theoretical modeling, New Alchemy provides technology, token game theory, smart contracts, security audits, and ICO advisory to the most innovative startups worldwide. Get in touch with us at Hello@NewAlchemy.io