SMART CONTRACT SECURITY AUDIT: MEMBRANA

MEMBRANA
Membrana is a decentralized platform which redefines trust, it aspires to bring investors and traders in a secure network where both the parties can mutually benefit from blockchain-protected contracts. The transparency and security offered by Membrana platform instigate trust which eventually benefits both parties. The main objective of Membrana.io is to bring much-needed transparency in high-risk investments by making trust management more profitable and secure.  

Complications in Existing Trust Management System
At present, there is no convenient and safe tool available in the market that can be effectively used for the conclusion of contracts between investors and traders. When trading cryptocurrencies, there is often lack of trust between the parties as these contracts are usually active in words, forums, and chats. As a result, this leads to fraudulent acts and lack of transparency with increased risks for investors and traders.

Membrana’s Solution
Membrana platform makes room for the investors to give traders an opportunity to manage funds, however not to possess these funds, thereby making it secure and transparent. Here, traders do not own these funds as the smart contracts are executed when certain criteria are met in the blockchain network. Membrana.io guarantees ethical trading of assets along with the fulfillment of contract terms by diminishing the lack of trust among the parties as well as the risk that comes along in the form of violation of contractual terms.

Teachracers’ team was approached to perform a step-by-step Security Audit for Membrana.io’s  Smart Contract. Below is the report and highlighted vulnerabilities which were identified and further addressed:

Manual Audit

Style Guide Violations
Not necessary to resolve but increase readability and enforces style guide standards accepted by the global community. An example image from the contract has been added for each violation but similar violations may repeat themselves within the contract.

  1. Usage of obsolete pragma versions of solidity cited within the contract, instead the latest stable version 0.4.20, commit 3155dd80, should be used.SMART CONTRACT SECURITY AUDIT: MEMBRANA
  2. It is a standard practice to use the same pragma version throughout the contracts. Instead different pragma versions were cited in different contracts.SMART CONTRACT SECURITY AUDIT: MEMBRANA
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  3. Pragma should be locked before deployment in every contract. (ref) (For example, 0.4.18 is locked whereas ^0.4.18 is not).
  4. Top level declarations in solidity should have two blank lines before them. (ref)
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  5. Solidity recommends to surround function declarations with a single blank line within a contract. But, within the contract MercatusDeals.sol no such recommendation is followed. (ref)
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  6. No indentation strategy is followed within the code, this makes it very difficult to read. Please refer to Solidity Official Style Guideindentation recommendation, which proposes 4 spaces without mixing it with tabs for indentation
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  7. Function order is incorrect at multiple instances within the contracts, the correct
    order is as follows: (ref)
    Constructor
    -Fallback function (if exists)
    – External
    – Public
    – Internal
    – Private
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  8. Length of a line must not exceed 120 characters, but line length for declaration of makeDeal function equals 220 characters. Similarly getSplit function is 121 characters long.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  9. Definitions within contracts/libraries must be separated by 1 line. (ref)
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  10. Enum names must be in CamelCase, therefore enum state should be changed to State, and enum currencyType should be changed to CurrencyType.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  11. Event names must be in CamelCase, therefore event name spawnInstance should be changed to SpawnInstance.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  12. Expression indentation is incorrect, we should use same number of spaces on both side of an operator.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  13. Comma must be separated by next element, with a space
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  14. Expression indentation is incorrect, required no spaces after ( and before ), in the following expression:
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
    Similar violation also present here:
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  15. While declaring a function there should be a space between “)” and “{”. Similar mistake can be cited at multiple instances.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  16. Visibility of all functions should be specified, failing which it is defaulted to public. (ref)
    Other than this, open bracket, i.e. “{” must be indented by other constructions by space.SMART CONTRACT SECURITY AUDIT: MEMBRANA
  17. Wrong spacing cited during the following function declarations:
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
    There should be a single space between “)” and “{“.
  18. There should be a space between if and “()”. (ref)
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  19. Visibility modifiers should be first among the list of modifiers in a function declaration. Therefore in function makeDeal, public should appear before payable

    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  20. Visibility of all state variables should be explicitly specified, failing which it is defaulted to ’internal’.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA

Minor Considerations

  1. be” is an ambiguous address parameter name, similarly onlyBe becomes an ambiguous modifier name, it should be renamed to something more relevant and easy to understand.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  2. In struct Deal, variable start and deadline represent time intervals, so it would be more appropriate to rename them to startTime and endTime.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
    Similarly, it would be better to rename parameters investor and trader to investorName and traderName.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  3. Modifier can be updated from constant to view in getState and getStart function.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  4. Re-entrancy is on of the major considerations of Solidity, you need to follow a specific code structure in-order to avoid it, and such standard practises should be respected, one of them is to make all the state changes before you use call.value, send or transfer functions, a violation against this rule can be cited within the contracts in functions setHalted and setFinished, we can solve it by changing the currentState of a deal before transfer function is used, in case transfer fails, it is designed to automatically revert back the state changes already happened in the function.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  5. Parameter amount represents the amount in terms of wei, a sub unit of ether. So, it should be renamed to weiAmount for clarity.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  6. The initial state of enums is defaulted to their first state, therefore default state of every instance of state enum will be paid, and currencyType enum will be USDT. a default state for both enums, representing not-initialized should be used.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  7. It is a good practice to add events in important functions that change state. It makes it easier for the front end application to be built around the smart contract. Only one event is fired from the current contract, it is a suggestion to add more.

Major Considerations

  1. Variable dealsCount is declared, but it is not used within the MercatusDeals contract.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  2. be” parameter’s address is hardcoded into the contracts and then onlyBe modifier is used in many functions, hardcoding owner addresses is not a good practise and instead Ownable contract should be inherited within MercatusDeals contract, this will also enable provisions to transfer ownership and other maintenance functions.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  3. In the MercatusDeals contract, constructor and fallback functions are payable. This means they can accept Ethers while being called, we don’t see any benefit in making the constructor to the said contract payable, i.e. sending money to the contract while deploying it. Also making these two functions payable, exposes a critical issue. These functions allow the contract to accept ethers which cannot be handled by any function present in the contract, therefore Ethers sent to the contract using these two functions will be stuck in the contract forever. There is a different function in the contract namely makeDeal, it is handling the Ethers sent to it using functions setHalted and setFinished, but same cannot be said for Ethers from constructor and fallback.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  4. Possible Integer Underflow/Overflow can happen in the contract, many +, -, *, / operators are used carelessly, which can trigger such a situation. We advise to use safeMath library from zeppelin, already present within the client repository to remove this vulnerability.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  5. Array deals is used within contract, instead a mapping should be used, this is because arrays consume more gas while creation/updation as compared to a mapping.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  6. No address(0) checks present within makeDeal function to check _traderAddress and _investorAddress.
  7. Parameter _amount is not needed within makeDeal function, already available msg.value parameter is enough to suffice it’s use.
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  8. _duration in the makeDeal() function must be passed in seconds unit rather than in days. Because it will not be possible to create a deal with duration which is not a multiple of days. A deal with duration of 5.5 days will not be possible. Using seconds will be more flexible.
  9. In makeDeal() function the use of offer variable  is not clear. It is not being saved anywhere and is being used only in SpawnInstance event. Same is the case with msg.sender.
  10. In makeDeal() function it must be checked that msg.value is not equal to zero. It does not make sense to make a deal with zero value.
  11. A kill() function must be added to this smart contract with modifier onlyBe. As this contract stores Ethers at its address it is very important that the owner could call the selfdestruct() function and save his Ethers in case of an emergency. If no kill function all the ethers will be stuck at contract address with no use.

Automated Audit

Mythril

  1. Warning
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  2. Warning 2:
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  3. Warning 3:
    SMART CONTRACT SECURITY AUDIT: MEMBRANA
  4. Warning
    SMART CONTRACT SECURITY AUDIT: MEMBRANA

Oyente:
SMART CONTRACT SECURITY AUDIT: MEMBRANA

Securify
Unable to compile the contract and results in an error.

Remix
Successfully deployed contract on Ropsten network and in JavaScript VM. You can see the transaction here. Gas used by transaction was: 10,30,790. Successfully ran makeBet function on Ropsten, cannot test functions with onlyBe modifier because be is hardcoded.

Summary
The code quality needs to be improved and there are many minor/major issues that are important to be addresses before contracts are deployed on Ethereum mainnet, many contracts which can solve some of the above mentioned problems are already present within client’s repository, but not used within the contracts.

Conclusion
We performed our audit as perto the procedure described above. Our team also carefully checked Smart Contracts logic and compared it with the one described in the Membrana white paper. As a result, few medium,minor and major vulnerabilities were found and notified.

 

 

February 27th, 2018|Security Audit|