Skip to content

Instantly share code, notes, and snippets.

@Arachnid Arachnid/melonport-audit-draft.md Secret
Last active Dec 15, 2017

Embed
What would you like to do?

Section 1 - Table of Contents

Section 2 - Introduction

This is a report of an audit performed by Martin Holst Swende (Dyno Security AB) and Nick Johnson (Arachnid Labs Ltd).

Section 3 - Summary

The Melonport protocol implementation is a rather complex set of contracts, with non-trivial interactions between them. We have found the code to be reasonably well written, but containing a high number of bugs. We do not consider the codebase production ready.

The findings we have made indicate that there is a lack code coverage for the tests, which should be further extended to cover both expected and unexpected flows; both success-paths and failure-paths. Both unit-tests and more high-level end-to-end tests are needed.

An example of an end-to-end test would be to validate that all owned assets are accounted for at all times; after the creation of a Fund, some interaction (making/taking orders, participants entering and leaving etc). A more robust testsuite could also be used to validate that the required gas levels for different scenarios are within acceptable bounds, and make it easier to perform gas-optimizations.

Section 4 - Background and scope

The audit was based on the code available at the Melonport repository, at the commit -hash 576bdbf5ec9be6f8097410e187d4d1b4374895f2.

The following files were not included in the scope:

  • SimpleMarket.sol
  • Governance.sol
  • exchange/thirdparty/*

This audit was based on manual code review, and no unit- or integration tests have been performed as a part of this audit. The audit was focused on finding security vulnerabilities; issues which could have an adverse on the platform itself or its participants; users, managers and administrators.

The issues have been rated according to OWASP rating methodology, by impact and likelihood, into a final severity rating with the following steps:

  • Note
  • Low
  • Medium
  • High
  • Critical

Section 5 - Architecture

The core of the Melon Protocol is the Fund. Users can invest in the fund, and redeem their shares at a later time. The investment can take the shape of either supplying MLN tokens or the mix-bag of tokens that make up the fund -- and vice versa when redeeming.

Both buying and redeeming is based on placing a request, which can be executed by any caller. There is an incentive-mechanism to reward callers for processing requests.

A Fund can be shutdown, at which point it should still be possible for participants to redeem their holdings.

!TODO

  • Bullet-list of inheritance

5.1 Authorisation model

These are the roles:

  • Users buy and redeem fund shares. If fund closes, they can redeem their assets.
  • manager. A manager manages a fund, and can rebalance the fund portfolio via making and taking orders through and exchange. The manager can shut down the fund.
  • owner. The creator of the fund, ostensibly Version. The owner can shut down the fund.
  • Governance, which controls the Version contract. This is a multi-owned governance contract based on ds-governance (not part of the scope). Governance can replace the Version contract with a newer version, and shut down funds controlled by the Version instance.
  • Datafeed. The datafeed contract provides quotes for the Fund. The entity who submits data to the pricefeed is, needless to say, very powerful, being the ultimate source for 'market price'.

Section 6 - Findings

6.1 High

6.1.1 H2: EtherToken can be emptied (EtherToken.sol)

Since the EtherToken is a PreminedAsset, it's possible that the total credited balance exceeds this.balance. This means that it's not a trustless version; it will only function correctly if the premined value is either not collected, or added prior to deployment.

Notably, the preminedAmount is set to 10**28, or 10 billion ether; which basically makes the EtherToken fully controlled by the owners, since the number of premined ethertokens will overshadow the possible influx of actual and available ether within the contract.

OBS The Melonport developers have stated that the mainnet-version of EtherToken will not be premined -- the reason for premining was for testing purposes.

6.1.1.1 Recommendations

We recommend that EtherToken is not a PreminedAsset when deployed on the mainnet.

6.2 Medium

6.2.1 M1: Insecure custom error-handling (Fund.sol)

In Fund.sol, a custom error-handling-scheme has been implemented. This error-handling scheme is not robust, and cause unintended consequences.

6.2.1.1 RedeemOwnedAssets failure to catch error

The redeemOwnedAssets seems to assume that transfer will return a boolean status. Most implementations instead performs a throw in case of failure, meaning that the shutDownAndLogError will not be called.

            // Failed to send owed ownershipQuantity from fund to participant
            if (!ERC20(ofAsset).transfer(msg.sender, ownershipQuantity)) {
                return shutDownAndLogError("CRITICAL ERR: Transfer of an asset failed!");
            }

6.2.1.2 redeemOwnedAssets failure to revert on error

Furthermore, there may be unintended consequences if the transfer does not throw on failure. If that happens, the Fund will be shutDown. At this point, the method annihilateShares has already been called on behalf of the msg.sender, effectively terminating his share in the Fund as if the transfer had been successfull.

        annihilateShares(msg.sender, shareQuantity); // Annihilate shares before external calls to prevent reentrancy
        // Transfer ownershipQuantity of Assets
        for (uint i = 0; i < module.datafeed.numRegisteredAssets(); ++i) {
            address ofAsset = address(module.datafeed.getRegisteredAssetAt(i));
            uint assetHoldings = ERC20(ofAsset).balanceOf(this);
            if (assetHoldings == 0) continue;
            uint ownershipQuantity = assetHoldings // ownership percentage of participant of asset holdings
                .mul(shareQuantity)
                .div(participantsTotalSupplyBeforeRedeem);

            // Less available than what is owed - Eg in case of unreturned asset quantity at EXCHANGE address
            if (isLessThan(assetHoldings, ownershipQuantity)) {
                return shutDownAndLogError("CRITICAL ERR: Not enough assetHoldings for owed ownershipQuantitiy");
            }

            // Failed to send owed ownershipQuantity from fund to participant
            if (!ERC20(ofAsset).transfer(msg.sender, ownershipQuantity)) {
                return shutDownAndLogError("CRITICAL ERR: Transfer of an asset failed!");
            }
        }

One case where this may potentially be triggered by a malicious user is when ownershipQuantity is 0. Some ERC20 implementations treats 0-value transfers as failures.

6.2.1.3 takeOrder failure to revert on error

During takeOrder, it's possible that the approveSpending operation succeeds, but the takeOrder fails. If that's the case, the method returns without reverting state changes.

        uint spendQuantity = quantity.mul(order.buyQuantity).div(order.sellQuantity);

        if (!approveSpending(order.buyAsset, spendQuantity)) {
            return logError("ERR: Could not approve spending of spendQuantity of order.buyAsset");
        }

        bool success = exchangeAdapter.takeOrder(EXCHANGE, id, quantity);

        if (isFalse(success)) {
            return logError("ERR: Exchange Adapter: Failed to take order");
        }

Thus, the sender has now approved the EXCHANGE to spend spendQuantity on the ERC20 asset order.buyAsset. There are two consequences:

  • The user may be unaware of an existing approval for the exchange to spend funds on his behalf.
  • A second attempt will most likely fail, since most ERC20 implementations do not allow re-setting approve to a non-zero value if there already exists a non-zero value approval.

6.2.1.4 mergeOrder failure to revert on error

The makeOrder tries to approve spending on behalf of the exchange:

        if (!approveSpending(sellAsset, sellQuantity)) {
            return logError("ERR: Could not approve spending of sellQuantity of sellAsset");
        }

The approveSpending calls approve against the token, with the EXCHANGE as beneficiary:

    function approveSpending(address ofAsset, uint quantity) internal returns (bool success) {
        success = ERC20(ofAsset).approve(EXCHANGE, quantity);
        SpendingApproved(EXCHANGE, ofAsset, quantity);
    }

However, this may fail if it had already been set to something else, necessitating the approval to be set to 0 prior to this. Doing so may prove difficult, as it requires arriving at the same line of code, but with quantity at 0.

Cancelling an order does not set back the approval to 0, so basically making an order and then cancelling would lock the trading on that asset-pair (in case approval-reset is implemented on that ERC20 token)

6.2.1.5 Recommendations

Perform full rollback (using e.g. require) when errors occur.

6.2.2 M2: quantityHeldInCustodyOfExchange returns wrong (Fund.sol)

openMakeOrderId is not set to nonzero anywhere, so this method will always return 0.

Additionally, the method takes an address ofAsset which is totally ignored by the method.

6.2.3 M3: convertUnclaimedRewards invariant breach (Fund.sol)

When converting rewards into shares, there is a break of an invariant.


        // Convert unclaimed rewards in form of ownerless shares into shares which belong to manager
        uint shareQuantity = totalSupply.mul(unclaimedRewards).div(gav);
        totalSupply = totalSupply.sub(shareQuantity); // Annihilate ownerless shares
        addShares(owner, shareQuantity); // Create shares and allocate them to manager

Consider the following example:

  • totalSupply is 100, with 10 holders each holding 10 shares
  • shareQuantity returns 5 shares
  • totalSupply becomes 95
  • addShares(owner, 5) sets totalSupply back to 100, and allocates 5 shares to owner

The result being: 10 holders with 10 shares each, and owner holding 5 shares, totalling 105 shares. However, the totalSupply is only 100 shares.

This breaks the invariant that the sum of share-holdings equals the totalSupply.

6.2.3.1 Recommendations

Investigate how the underlying model should be modified to ensure that the total supply (credit) is equal to the sum of holdings (debit).

6.2.4 M4: Presence and price of MLN in the asset feed is not enforced (Fund.sol)

Fund implicitly assumes that the DataFeed contains MLN tokens as an asset, with a price of 1, but this is not enforced or special-cased anywhere.

If MLN is not present, then GAV and NAV calculations will be incorrect, and users calling redeemOwnedAssets will be shortchanged, receiving only the other assets held by the contract, but not the MLN tokens.

If MLN is present, but has a value other than 1, the fund will give the user either too many or too few MLN tokens when they call redeemOwnedAssets, and other unexpected behaviour is likely to occur in addition.

6.2.4.1 Recommendation

We recommend either special-casing MLN tokens so that the feed always returns them, changing all relevant calculations to add the quantity of MLN tokens held, or require MLN to be submitted with a price of 1 in all feed updates.

6.3 Low

6.3.1 L1: Missing token-handling (EtherToken.sol)

ERC20-funds is sometimes mistakenly sent to contracts. Unless there's some way to recover those funds (ability to call transfer on the token contract), the tokens become stuck forever.

The following contracts do not contain functionality to recover lost ERC20-tokens:

  • EtherToken

6.3.1.1 Recommendations:

Have a recoverToken method which allows the deployer/owner to recover tokens from the contract.

6.3.2 L2: Methods marked 'constant' modify storage (Fund.sol)

The method quantityHeldInCustodyOfExchange potentially writes to storage, despite being marked constant.

    function quantityHeldInCustodyOfExchange(address ofAsset) constant returns (uint) {
        if (openMakeOrderId == 0) return 0;
        var ( , , sellQuantity, ) = exchangeAdapter.getOrder(EXCHANGE, openMakeOrderId);
        if (sellQuantity == 0) openMakeOrderId = 0;
        return sellQuantity;
    }

The method in question is called from the following methods (also marked constant):

  • function calcGav() constant
    • called from performCalculations
      • called from calcSharePrice

6.3.3 L3: Incorrect accounting of funds held in exchange (Fund.sol)

/// @notice Calculates gross asset value of the fund
/// @dev Decimals in assets must be equal to decimals in PriceFeed for all entries in Universe
/// @return gav Gross asset value denominated in [base unit of melonAsset]
function calcGav() constant returns (uint gav) {
    for (uint i = 0; i < module.datafeed.numRegisteredAssets(); ++i) {
        address ofAsset = address(module.datafeed.getRegisteredAssetAt(i));
        uint assetHoldings = uint(ERC20(ofAsset).balanceOf(this)) // Amount of asset base units this vault holds
            .add(quantityHeldInCustodyOfExchange(ofAsset));
        uint assetPrice = module.datafeed.getPrice(ofAsset);
        uint assetDecimals = module.datafeed.getDecimals(ofAsset);
        gav = gav.add(assetHoldings.mul(assetPrice).div(10 ** uint256(assetDecimals))); // Sum up product of asset holdings of this vault and asset prices
        PortfolioContent(assetHoldings, assetPrice, assetDecimals);
    }
}

calcGav calls quantityHeldInCustodyOfExchange with the asset address, and adds its value to the total holdings of that asset. However, as previously noted, quantityHeldInCustodyOfExchange does not respect the argument passed to it, and will always return the same value. If this value is non-null, it will be added to every asset.

6.3.4 L4: Use of 'toggle' type functions (Fund.sol)

function toggleSubscription() external pre_cond(isOwner()) { isSubscribeAllowed = !isSubscribeAllowed; }
function toggleRedemption() external pre_cond(isOwner()) { isRedeemAllowed = !isRedeemAllowed; }

Functions to toggle states are dangerous, as they can lead to accidental state changes, such as in cases where the caller does not check the current state, accidentally submits a transaction twice, or two callers submit the same transaction.

Instead, we recommend replacing these with functions that take a bool and update the flag accordingly.

6.3.5 L5: ExecuteRequest silently ignores unrecognised request types (Fund.sol)

    if (
        request.requestType == RequestType.subscribe &&
        costQuantity <= request.giveQuantity
    ) {
        assert(MELON_CONTRACT.transferFrom(request.participant, msg.sender, request.incentiveQuantity)); // Reward Worker
        assert(MELON_CONTRACT.transferFrom(request.participant, this, costQuantity)); // Allocate Value
        createShares(request.participant, request.shareQuantity); // Accounting
    } else if (
        request.requestType == RequestType.redeem &&
        request.receiveQuantity <= costQuantity
    ) {
        assert(MELON_CONTRACT.transferFrom(request.participant, msg.sender, request.incentiveQuantity)); // Reward Worker
        assert(MELON_CONTRACT.transfer(request.participant, request.receiveQuantity)); // Return value
        annihilateShares(request.participant, request.shareQuantity); // Accounting
    }

In the event that executeRequest is called with a request type that is not explicitly catered for, or with a valid request type but with a costQuantity that is too high, the request is silently ignored and the function returns without error.

6.3.5.1 Recommendation

We recommend reverting in the case that a valid request cannot be executed, and asserting/throwing in the event that an invalid request type is found.

6.3.6 L6: Unexpected behaviour if a fund contains its own tokens (Fund.sol)

There are no checks to prevent a fund buying its own tokens. This would likely lead to unexpected behaviour.

6.3.6.1 Recommendations

We recommend either forbidding this if possible, or excluding such assets from calculations in calcGav.

6.3.7 L7: Allocates insufficient storage for IPFS hash (AssetRegistrar.sol)

AssetRegistrar allocates a bytes32 for IPFS hashes, but IPFS hashes use the multihash standard, and are larger than 32 bytes.

6.3.7.1 Recommendation

We recommend using the type bytes, instead.

6.3.8 L8: Possibly fail-prone price feed (DataFeed.sol)

The method update of a price-feed does not enforce that all assets are updated when update is called.

A fund manager may want to update prices in two discrete calls, in order to keep the gas levels low. However, doing so means that the dataHistory for the index LastUpdateId() : nextUpdateId - 1 is not set, which will result in the price to effectively return 0.

    function getPrice(address ofAsset)
        constant
        pre_cond(isDataSet(ofAsset))
        pre_cond(isDataValid(ofAsset))
        returns (uint dataFeedPrice)
    {
        return dataHistory[getLastUpdateId()][ofAsset].price;
    }

6.3.8.1 Recommendations

Ensure that the behaviour of update when passed partial data is consistent with expectations.

6.3.9 L9: existsData will always return false (DataFeed.sol)

function isDataSet(address ofAsset) internal returns (bool) { return dataHistory[getLastUpdateId()][ofAsset].timestamp > 0; }

/// @notice Gets asset specific information
/// @dev Asset has been initialised
/// @return valid Whether data is valid or not
function isValid(address ofAsset)
    constant
    pre_cond(isDataSet(ofAsset))
    returns (bool valid)
{
    return now - dataHistory[getLastUpdateId()][ofAsset].timestamp <= VALIDITY;
}

/// @notice Checks whether data exists for a given asset pair
/// @dev Prices are only updated against QUOTE_ASSET
/// @param sellAsset Asset for which check to be done if data exists
/// @param buyAsset Asset for which check to be done if data exists
/// @return exists Whether data exists for a given asset pair
function existsData(address sellAsset, address buyAsset)
    constant
    returns (bool exists)
{
    return
        isValid(sellAsset) && // Is tradable asset (TODO cleaner) and datafeed delivering data
        isValid(buyAsset) && // Is tradable asset (TODO cleaner) and datafeed delivering data
        (buyAsset == QUOTE_ASSET || sellAsset == QUOTE_ASSET) && // One asset must be QUOTE_ASSET
        (buyAsset != QUOTE_ASSET || sellAsset != QUOTE_ASSET); // Pair must consists of diffrent assets
}

isValid checks dataHistory for the passed in asset, and existsData enforces this be true for both passed in assets. However, the check also requires that one of the passed in assets be the QUOTE_ASSET, which is not enforced to be one of the assets provided in an update.

6.3.9.1 Recommendations

Either this check should be changed so as to not require (meaningless) data for QUOTE_ASSET, or update should be changed to require that QUOTE_ASSET always be supplied.

The last line of the check in existsData can also be more succinctly and clearly formulated as buyAsset != sellAsset.

6.3.10 L10: managerToFund is overwritten if a manager creates a second fund (Version.sol)

In setupFund, managerToFund[msg.sender] is set to the newly created fund. This implicitly supports only one fund per manager, and subsequent fund creations will silently overwrite previous ones.

    function setupFund(
        string withName,
        address ofReferenceAsset,
        uint ofManagementRewardRate,
        uint ofPerformanceRewardRate,
        address ofParticipation,
        address ofRiskMgmt,
        address ofSphere,
        uint8 v,
        bytes32 r,
        bytes32 s
    )
        pre_cond(termsAndConditionsAreSigned(v, r, s))
        pre_cond(notShutDown())
        pre_cond(!fundNameExists[withName])
    {
        address fund = new Fund(
            msg.sender,
            withName,
            ofReferenceAsset,
            ofManagementRewardRate,
            ofPerformanceRewardRate,
            MELON_ASSET,
            ofParticipation,
            ofRiskMgmt,
            ofSphere
        );
        listOfFunds.push(fund);
        fundNameExists[withName] = true;
        managerToFunds[msg.sender] = fund;
        FundUpdated(getLastFundId());
    }

6.3.10.1 Recommendations

We recommend either changing the type of managerToFund to mapping(address=>address[]), or adding an explicit check to prevent one manager creating multiple funds. The rating has been set to Low since the current codebase does not appear to use the GetFundByManager method.

6.3.11 L11: Non-overflow-safe math operations (Rewards.sol)

The calculations within rewards.sol does not use safeMath.sol, to protect against overflows.

    /// @dev Post Reward denominated in referenceAsset
    function managementReward(
        uint managementRewardRate,
        uint timeDifference,
        uint gav,
        uint divisorFee
    )
        constant
        returns (uint)
    {
        uint absoluteChange = timeDifference * gav;
        return absoluteChange * managementRewardRate / divisorFee;
    }

    /* Function invariant
     *  for deltaDifference == 0 => returns 0
     */
    /// @dev Post Reward denominated in referenceAsset
    function performanceReward(
        uint performanceRewardRate,
        int deltaPrice, // Price Difference measured agains referenceAsset
        uint totalSupply,
        uint divisorFee
    )
        constant
        returns (uint)
    {
        if (deltaPrice <= 0) return 0;
        uint absoluteChange = uint(deltaPrice) * totalSupply;
        return absoluteChange * performanceRewardRate / divisorFee;
    }

6.3.11.1 Recommendation

Use safemath library

6.3.12 L12: Violation of ERC20 with approve requirement (ERC20.sol)

The approve function throws if the caller attempts to change an approval unless either the previous approval amount or the new approval amount is zero. This is intended to prevent a known edge case with ERC20 approvals, but results in violating ERC20.

The approvals race condition does not affect other contracts, which can read and adjust approvals in a single atomic operation, and calling contracts may be unaware of this check, which violates the interface specification laid out in ERC20. As a result, these contracts may be rendered unusable with this token.

6.3.13 Recommendation

We recommend leaving out this check, and making it the responsibility of user software that issues approvals to responsibly zero them out and check them before reauthorisation.

6.3.14 L13: Use of getName, getSymbol, getDecimals instead of name, symbol, decimals (AssetInterface.sol)

ERC20 prescribes optional functions name, symbol, and decimals, but AssetInterface implements these prefixed with get. As a result, these variables will not be accessible to contracts that expect the standardised names to exist.

6.3.14.1 Recommendations

We recommend renaming these functions to match the names in ERC20.

6.3.15 L14: Owned offers no provision for changing ownership (Owned.sol)

Unlike many implementations of the Owned pattern, this implementation provides no mechanism for changing ownership.

6.3.15.1 Recommendations

We recommend adding one, to cater for situations such as key rotation or the need to migrate to a new multisig wallet.

6.4 Note

6.4.1 N1: constant method raise Events (Fund.sol)

The method calcGav is a constant function, but raises events. This should be the responsibility of the caller, where appropriate, as it is not an expected side-effect of calling a constant function.

6.4.2 N2: Dead code in cancelOrder (Fund.sol)

The cancelOrder method calculates an assetPair which is not used in the subsequent code. It's unclear what the intention was.

    function cancelOrder(uint id)
        external
        pre_cond(isOwner() || isShutDown)
        returns (bool err, string errMsg)
    {
        Order memory order = orders[id];

        bytes32 assetPair = sha3(order.sellAsset, order.buyAsset);
        bool success = exchangeAdapter.cancelOrder(EXCHANGE, order.exchangeId);

        if (isFalse(success)) {
            return logError("ERR: Exchange Adapter: Failed to cancel order");
        }

        OrderUpdated(id);
    }

6.4.2.1 Recommendation

Remove the unused code.

6.4.3 N3: Redeem assets needlessly complex (Fund.sol)

The calculations in redeemOwnedAssets seem needlessly complex; it does not need to calculates the total value of everything. It should be sufficient to just refund the user, for each asset, the proportional value of each token.

6.5 N4: executeRequest incentive flow (Fund.sol)

The executeRequest has an incentive-mechanims to incentivise callers from executing requests. A shareholder who wishses to e.g. redeem shares into MLN tokens can set a incentiveQuantity; a number of tokens to be sent to the msg.sender as payment for making the transaction.

        if (
            request.requestType == RequestType.subscribe &&
            costQuantity <= request.giveQuantity
        ) {
            assert(MELON_CONTRACT.transferFrom(request.participant, msg.sender, request.incentiveQuantity)); // Reward Worker
            assert(MELON_CONTRACT.transferFrom(request.participant, this, costQuantity)); // Allocate Value
            createShares(request.participant, request.shareQuantity); // Accounting
        } else if (
            request.requestType == RequestType.redeem &&
            request.receiveQuantity <= costQuantity
        ) {
            assert(MELON_CONTRACT.transferFrom(request.participant, msg.sender, request.incentiveQuantity)); // Reward Worker
            assert(MELON_CONTRACT.transfer(request.participant, request.receiveQuantity)); // Return value
            annihilateShares(request.participant, request.shareQuantity); // Accounting
        }

In the second case, it might be a better idea to switch order of the two transfers, so that the incentive quantity can be paid from the newly redeemed MLN-tokens.

6.5.1 N5: Use of unnecessary predicates (Fund.sol)

Fund contains a number of predicates that test simple conditions.

function isZero(uint x) internal returns (bool) { x == 0; }
function isFalse(bool x) internal returns (bool) { return x == false; }
function isPastZero(uint x) internal returns (bool) { return 0 < x; }
function isLessThan(uint x, uint y) internal returns (bool) { return x < y; }
function notShutDown() internal returns (bool) { return !isShutDown; }
function balancesOfHolderLessThan(address ofHolder, uint x) internal returns (bool) { return balances[ofHolder] < x; }
function isVersion() internal returns (bool) { return msg.sender == VERSION; }

These predicates are used in DBC modifiers to test conditions, such as pre_cond(isPastZero(totalSupply)).

However, in many cases these modifiers only serve to obfuscate the control flow and make it harder to follow. In the example above, pre_cond(totalSupply > 0) would be simpler and easier to read.

6.5.1.1 Recommendations

We recommend replacing these functions with the expressions they represent whenever possible.

6.5.2 N6: Underuse of modifiers (Fund.sol)

Many functions, such as requestSubscription, requestRedemption, executeRequest and cancelRequest contain checks that could be more succinctly expressed as preconditions.

6.5.2.1 Recommendations

We recommend rewriting these as modifiers or preconditions for clarity.

6.5.3 N7: Duplicate ABI-exposed values (Fund.sol)

function getName() constant returns (string) { return NAME; }
function getSymbol() constant returns (string) { return SYMBOL; }
function getDecimals() constant returns (uint) { return DECIMALS; }
function getCreationTime() constant returns (uint) { return CREATED; }
function getBaseUnits() constant returns (uint) { return MELON_IN_BASE_UNITS; }

A number of functions return that simply return values also exposed via public variables. We recommend making these variables private to avoid confusion around the correct part of the interface to use, and to reduce the size of the ABI.

6.6 N8: Unnecessarily exposed methods and confusing naming (Fund.sol)

calcGav, calcUnclaimedRewards, calcNav and calcValuePerShare are internal methods that are part of the accounting process, and shouldn't be exposed publicly.

calcUnclaimedRewards returns managementReward, performanceReward and unclaimedRewards, but the latter is simply the sum of the first two. If the individual arguments are required by callers, the third should be omitted, and callers can add the two values together to obtain it.

performCalculations is poorly named, and does not reflect its purpose.

Finally, calcSharePrice simply extracts a single element from the performCalculations return tuple, and is redundant.

6.6.1 N9: Orders are never cleaned up (Fund.sol)

Orders are added to an array as they're placed, but are never deleted to free storage when they are cancelled or filled. In the interest of being a good 'blockchain citizen', we recommend deleting obsolete data.

6.6.2 N10: Incorrect data type for chain ID (AssetRegistrar.sol)

Chain IDs are integers, but AssetRegistrar specifies them as type bytes32.

6.6.2.1 Recommendation

We recommend changing this to uint.

6.6.3 N11: Unnecessary notRegistered function (AssetRegistrar.sol)

function notRegistered(address a) internal constant returns (bool) { return information[a].exists == false; }
function isRegistered(address ofAsset) constant returns (bool) { return !notRegistered(ofAsset); }

This would be simpler to follow and understand if isRegistered checked information[a].exists, and current callers of notRegistered were adapted to negate the result.

6.6.4 N12: Needlessly complex check for same-block update. (DataFeed.sol)

The function update seems overly complex.

    function update(address[] ofAssets, uint[] newPrices)
        pre_cond(isOwner())
        pre_cond(ofAssets.length == newPrices.length)
    {
        uint thisId = nextUpdateId;
        for (uint i = 0; i < ofAssets.length; ++i) {
            if(thisId > 0)  // prevent multiple updates in one block
                require(dataHistory[thisId - 1][ofAssets[i]].timestamp != now);
            dataHistory[thisId][ofAssets[i]] = Data({
                timestamp: now,
                price: newPrices[i]
            });
        }
        lastUpdateTimestamp = now;
        DataUpdated(thisId);
        nextUpdateId++;
    }

6.6.4.1 Recommendations

Since the lastUpdateTimestamp is available, a simpler solution would be to use require(lastUpdateTimestamp < now).

6.6.5 N13: getDataHistory makes unnecessary use of fixed-length arrays (DataFeed.sol)

/// @notice Returns data feed history in an blockchain node friendly way
/// @dev Uses an efficient bulk call
/// @param ofAsset Asset for which data history should be returned
/// @param withStartId Index at which history should be started, this is due to the limitation of non dynamic array size returns
/**
@return {
  "timestampArray": "Array of timestamps",
  "priceArray": "Array of prices"
}
*/
function getDataHistory(address ofAsset, uint withStartId)
    constant
    pre_cond(isHistory(withStartId))
    returns (uint[1024] timestampArray, uint[1024] priceArray)
{
    uint indexCounter;
    uint[1024] memory timestamps;
    uint[1024] memory prices;
    while (indexCounter != 1024 || withStartId + indexCounter < nextUpdateId) {
        timestamps[withStartId + indexCounter] =
            dataHistory[withStartId + indexCounter][ofAsset].timestamp;
        prices[withStartId + indexCounter] =
            dataHistory[withStartId + indexCounter][ofAsset].price;
        ++indexCounter;
    }
    return (timestamps, prices);
}

Assuming getDataHistory will be called by external callers, it is no longer necessary to define fixed-length arrays, as Solidity supports variable length arrays in return data.

If this call is expected to be used by onchain clients, they will soon also support variable length return data.

6.6.5.1 Recommendations

We recommend accepting a 'limit' parameter and outputting a variable length array to the caller.

6.6.6 N14: Wasted gas and storage for repeated storage of timestamp (DataFeed.sol)

All entries with the same index in dataHistory will have the same timestamp, but the timestamp is stored redundantly on each entry, wasting 20,000 gas and one storage slot per entry.

We recommend storing a single timestamp for each index in the history list instead.

6.6.7 N15: Use of obsolete throw (ERC20.sol)

ERC20 makes heavy use of statements of the type if(...) { throw; }. This syntax is now officially discouraged in favor of (preferably) require(...) or (otherwise) revert(...).

6.6.8 N16: No use of safeMath (ERC20.sol)

ERC20 does not use safeMath, instead performing manual overflow checks. Given this module is included elsewhere, it should be used here too, for simplicity.

6.7 Sphere.sol

The Sphere implementation in the audited code seemed to be incomplete and not fully functional, thus cannot be said to have been meaningfully audited.

6.7.1 No way to set EXCHANGE_ADAPTER or SUBSET

No functions exist to set EXCHANGE_ADAPTER or SUBSET, so these fields will always be 0.

6.7.2 No way to update EXCHANGE or DATAFEED

No functionality is provided to update EXCHANGE or DATAFEED after creation.

6.8 General notes

6.8.1 Use of constant instead of pure or view

constant is now deprecated by Solidity; functions should use view or pure instead.

6.8.2 Unnamed return values in interfaces

Several interfaces have functions that return tuples, but the interface does not name the elements of the return value. Naming the arguments would greatly aid readability of function signatures in interfaces.

6.8.3 Use of return values in functions intended to be called by transactions

Return data is not practical to retrieve from transactions. Many functions have return values, yet are clearly intended to be called by external callers.

Section 7 - Fixed issues

During the course of development, a number of issues have been reported to and fixed by the Melonport team.

  • AssetRegistrar.sol: No recovery from errors. Erroneous info cannot be updated.
    • Fixed, descriptive fields can now be updated
  • AssetRegistrar.sol: Unprotected asset registry, anyone can call register
    • Fixed, modifier pre_cond(isOwner()) has been added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.