The Abyss DAICO Smart Contract Audit

The Аbyss Team engaged New Alchemy™ to audit the smart contracts they created to support The Abyss digital distribution platform. We focused on identifying security flaws in the code or the design of the contracts that would have a security impact. Here are the results —

The audited code is located in the GitHub repository https://github.com/theabyssportal/DAICO-Smart-Contract repository and the version audited in this report is commit hash 6034f6f2ab899808f11f0ce5a175c11d9419f8b4.

New Alchemy’s audit was additionally guided by the following documents:

• The Abyss Whitepaper, version 1.6 (January 2018)
• The Abyss Frequently Asked Questions
• The Abyss Smart Contract Redeveloped in Compliance with DAICO Concept (2 February 2018)
• Less Than One Week Away From DAICO. Entering The Final Stretch (31 January 2018)

Update: The Abyss steam has followed our recommendations and updated The Abyss Token contract. New Alchemy has audited the new version, at commit 8d247055f5e70d7c20d07f7d711455345da2e192, to ensure that the previously identified issues were remedied. The fix for a new issue introduced while addressing the findings of the initial report was verified in commit 111ba433b32ca1241581af974d2e92a3cf7092cf.

The smart contract audit did not discover any issues of critical severity.

Several minor flaws were found, including one well-known issue. These include an ERC20 double-spend attack issue, rounding errors while calculating token refunds, token sale start and end dates that did not match documentation, and incomplete burning of tokens on project shutdown. After reviewing the original version of this report, the The Abyss team made changes to their documentation and contract implementations to fix the issues reported.

General Discussion

Three of the contracts in this project, SafeMath, Ownable, and ERC20Token, appear to be derived from the open-source OpenZeppelin project. Some of the issues noted below could be addressed by integrating more OpenZeppelin code: StandardToken contains mitigations for the ERC20 double-spend attack (unlike The Abyss' ERC20Token) and MintableToken prevents further token issuance after a flag is set (as opposed to The Abyss' Managedtoken).

Code clarity could be improved by inserting Natspec comments. Additionally, insert normal comments at places where it might not be clear what the code is doing such as magic numbers, multi-variable expression statements, or nested control blocks.

Most of the events do not have indexed parameters. This means clients will not be able to filter on desired topics, but instead will have to manually go through the log data for a contract.

These contracts do not make any attempt to address short-address attacks. The Abyss opted to follow the example of the OpenZeppelin project and various other contract developers by not mitigating these possible attacks, instead leaving mitigations to client code.

Minor Issues

Fixed: ERC20 double-spend attack

The standard ERC20 interface, implemented in ERC20Token, has a design flaw: if some user Alice wants to change the allowance that it grants to some other user Bob, then Alice can only check if Bob has already spent its allowance before issuing the transaction to change Bob's allowance. However, Bob can still spend the original allowance before the transaction changing the allowance is mined, allowing Bob to spend both the pre-change and post-change allowances. In order to have a high probability of successfully spending the pre-change allowance after the victim has verified that it is not yet spent, it may be necessary for the attacker to wait until the transaction to change the allowance is issued, then issue a spend transaction with an unusually high gas price to ensure that the spend transaction is mined before the allowance change. More detail on this flaw are available at https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/ and https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729.

There are two approaches that contracts can take to mitigate this flaw:

• In addition to the approve function mandated by the ERC20 standard, also provide increaseApproval and decreaseApproval functions that add or subtract to the existing allowances rather than overwriting them. This effectively moves the check of whether Bob has spent its allowance to the time that the transaction is mined, removing Bob's the ability of the recipient to double-spent. However, users must use this non-standard interface rather than approve; using approve remains open to abuse. This is approach is implemented by the current version of the OpenZeppelin library.
• Only allow approve to change allowances between zero and non-zero and don't allow multiple changes to a user's allowance in the same block. In order to change Bob's allowance, Alice must first set it to zero, wait until that transaction is mined, verify that the original allowance was not spent, then finally set the allowance to the new value. Requiring this sequence of operations by implementing restrictions in approve would violate the ERC20 standard, though users can still take this approach even without changes in the approve implementation.

Since both approaches are outside of the ERC20 standard, both approaches require user cooperation to work properly. Accordingly, The Abyss should provide documentation advising its users on how they should manage other users’ allowances.

Re-test results: The Abyss added increaseApproval and decreaseApproval implementations, derived from OpenZeppelin, to ManagedToken.

Fixed: Incorrect crowdsale start and end times

SALE_END_TIME in the AbyssToken and Crowdsale contracts is set to 1522540800 (April 1 00:00:00 UTC 2018), but TheAbyss FAQ says it should be 1522594800 (April 1 15:00:00 UTC 2018).

Similarly, SALE_START_TIME in the Crowdsale contract is 1517961600 (February 7 00:00:00 UTC 2018), but it should be 1518015600 (February 7 15:00:00 UTC 2018).

Re-test results: The sale start and end times are in flux. The Abyss has changed the dates in the contracts and in the FAQ several times and intends to ensure that they are accurate before the contracts are deployed.

Fixed: Refund leaves a share in the contract

Fund.enableRefund calculates refundTokenPrice as follows:

refundTokenPrice = safeDiv(this.balance, safeAdd(safeDiv(token.totalSupply(), 10**18), 1));

This appears to be calculating the price in Wei per whole token. It is unclear why 1 is being added to the denominator; if token.totalSupply happens to be divisible by 10**18, then this will have the effect of adding one to the supply, and if it is not divisible, then it rounds the token supply up to the nearest whole token. Either way, this calculation uses a total supply that is slightly too large, therefore making the value of refundTokenPrice slightly too small.

refundTokenPrice is later consumed in Fund.refundTokenHolder, which calculates the amount to refund to each user as follows:

uint256 refundAmount = safeDiv(safeMul(tokenBalance, refundTokenPrice), 10**18);

This takes the price per token in Wei (which is already too small), multiplies it by the number of token atoms held by the user, then finally divides it by the number of token atoms per token. The division truncates the result, so there are now two factors making refundAmount smaller than it should be. As a result, users may not be refunded the correct amount, leaving some balance in token.balance after all users have received their refunds.

Combining the two calculations yields the following:

refundAmount = tokenBalance * (this.balance / (1+(token.totalSupply / 10**18)) / 10**18

Removing the unexplained +1, the two conversions between atoms and whole tokens cancel, leaving a pure calculation in atoms:

refundAmount = tokenBalance * this.balance / token.totalSupply

Since this.balance should always be less than token.totalSupply (due to the sale ratio of 5000 tokens per Eth), the result of this.balance / token.totalSupply using integer division will always be zero; the conversions by 10**18 are presumably intended to compensate for this. However, performing the multiplication before the division should have the same effect without introducing any additional errors due to unnecessary divisions.

Note: As of commit 004140d7b0aacafbc5dec0189d07a34a50e3d633, this logic has been rearranged to move the whole calculation into Fund.refundTokenHolder with the +1 factor removed:

uint256 refundAmount = safeDiv(
    safeMul(tokenBalance, safeDiv(safeMul(this.balance, 10**18), token.totalSupply())), 
    10**18)

Every refund changes both this.balance and token.totalSupply, but mathematically, their ratio stays the same. Since this contract is actually dealing with a fixed-point representation (albeit with a large number of decimal places) rather than real numbers, it may still yield results that are slightly different for different users. A safer approach would be save copies the values of this.balance and token.totalSupply in Fund.enableRefund and use those in Fund.refundTokenHolderto calculate the refund amount rather than the current values:

uint256 refundAmount = safeDiv(
        safeMul(tokenBalance, this.savedRefundBalance), 
        this.savedRefundTokenSupply)

Re-test results: In commit 8d247055f5e70d7c20d07f7d711455345da2e192 the arithmetic remained as described above. However, The Abyss replaced the constants 10**18 with token.decimals(); since this constant is only 18 rather than 10**18, the product safeMul(this.balance, token.decimals() is likely to be smaller than token.totalSupply(), resulting in a quotient of zero that will propagate through the rest of the calculation. Since this results in all users receiving refunds of 0, this is a much more severe flaw than the original issue reported here.

As of commit 111ba433b32ca1241581af974d2e92a3cf7092cf, the refundAmount calculation has been changed again:

uint256 refundAmount = safeDiv(safeMul(tokenBalance, this.balance), token.totalSupply());

This version should yield correct results.

Fixed: Incomplete burning of tokens on project shutdown

In function Fund.enableRefund, tokens from only three of five wallets are burned. The two missing wallets are the advisors and referrals.

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.

ownership/MultiOwnable.sol

Line 11

Explicitly set visibility of state variable owners.

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

Line 31: setOwners

Fire an event since modifying ownership is a sensitive operation that users would be interested in.

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

Line 35

The check if (owners.length > 0) is unnecessary; it can't be negative and the loop on line 36 won't be entered if it is zero.

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

token/ERC20Token.sol

Lines 15, 25, 40: transfer, transferFrom, approve

These functions are vulnerable to the short-address attack.

Re-test results: The Abyss opted to follow the example of the OpenZeppelin project and various other contract developers by not mitigating these possible attacks, instead leaving mitigations to client code.

Token/LockedToken.sol

Line 40: addTokens

This function is vulnerable to the short-address attack.

Re-test results: The Abyss opted to follow the example of the OpenZeppelin project and various other contract developers by not mitigating these possible attacks, instead leaving mitigations to client code.

token/ManagedToken.sol

Consider using the BurnableToken contract from the Open Zeppelin project for “destroying” tokens.

Re-test results: The Abyss reports that the current code works as intended.

Lines 59, 76, 87, 100, 125, 141: transfer, transferFrom, issue, destroy, increaseApproval, decreaseApproval

These functions are vulnerable to the short-address attack.

Re-test results: The Abyss opted to follow the example of the OpenZeppelin project and various other contract developers by not mitigating these possible attacks, instead leaving mitigations to client code.

Lines 87–92: issue

This function can be called by the owner at any time, including after the crowdsale ends and token issuance should be complete. If this were to happen, the documented issuance procedure and ownership ratios would be violated. The Abyss reports that the owners of this contract will be other contracts that do not implement functionality to call this function after the crowdsale completes.

Re-test results: The Abyss implemented an additional check to ensure that this function cannot be called after token issuance is completed.

token/TransferLimitedToken.sol

Line 11

Constant variable limitTransfersPeriod should be in all caps style.

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

Lines 36, 45, 53

Consider implementing require(msg.sender == limitedWalletsManager) as a modifier.

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

Lines 52–55: setLimitState

limitedWalletsManager can enable limiting at any time, even after limitEndDate. This does not appear to be necessary.

Note: This issue appears to have been addressed as of commit 004140d7b0aacafbc5dec0189d07a34a50e3d633: limitedWalletsManager can now disable limiting at any time but cannot re-enable it.

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

Lines 74, 79, 85

Consider implementing canTransfer as a modifier.

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

voting/BaseVoting.sol

Line 124: onTokenTransfer

Create a new function variable instead of assigning to the function parameter amount.

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

AbyssToken.sol

Line 1

Missing version pragma.

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

Crowdsale.sol

Lines 58–59

The two state variables, BNB_HARD_CAP and BNB_MIN_CONTRIB, should follow the same pattern as other variables of the same purpose (e.g., HARD_CAP) and use the ether suffix. This makes the code clearer for reviewers.

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

Lines 268, 295

Consider using constants for numerator and denominator of telegram bonus. Similar to BNB_TOKEN_PRICE_NUM and BNB_TOKEN_PRICE_DENOM.

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

Fund.sol

Line 95: onCrowdsaleEnd

Redundant check in function body due to function modifier.

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

Lines 186, 201

The constant 18 on both lines should be replaced with token.decimals.

Re-test results: Rather than replacing the 18 with token.decimals, The Abyss replaced the whole expression 10**18, resulting in incorrect calculation of refund amounts, as described above. A subsequent change removed the constants altogether, fixing this issue.

Line 200: enableRefund

Consider using the tokenBalance function variable on line 197 instead of retrieving it again from the token.

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

OracleVoting.sol

Line 105

Since the contract tracks yesCounter, noCounter, and totalVoted, it would be a good idea to assert that totalVoted == yesCounter + noCounter. Otherwise, there isn't much point in storing all three variables; you could drop totalVotedand check that safeAdd(yesCounter, noCounter) >= minTotalVoted in isSubjectApproved.

Re-test results: The Abyss removed totalVoted and amended isSubjectApproved.

VotingManagedFund.sol

Lines 91–102: updateMinVotedTokens

If two or more polls run simultaneously, then minVotedTokensPerc will be updated based on whichever is finalized last, rather than some combination of the voting rate of the polls.

Re-test results: The Abyss reports that this in intended behaviour.

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