search

EthereumCommonwealth / Auditing

Ethereum Commonwealth Security Department conducted over 400 security audits since 2018. Not even a single contract that we audited was hacked. You can access our audit reports in the ISSUES of this repo. We are accepting new audit requests.
https://audits.callisto.network/
GNU General Public License v3.0
132 stars 34 forks source link

Need C++ Smart Contract Audited #671

Closed thi-fogworks closed 1 year ago

thi-fogworks commented 1 year ago

Audit request

The Data Mall Coin (DMC) is a new token issued by the Data Mall Coin Foundation. The purpose of the DMC is to:

  1. Create an efficient marketplace for decentralized storage, by allowing both buyers and sellers of decentralized storage to express how much decentralized storage they have/need and how much they are willing to pay/sell the decentralized storage for, and providing a fully decentralized matchmaking service to match buyers and sellers and help them enter into decentralized storage deals

  2. Provide proper incentives for both buyers and sellers of decentralized storage to store real (and not fake) decentralized data

  3. Give buyers the ability to verify that their decentralized data is being persisted, and provide on-chain arbitration and penalties if their data is not being persisted.

Source code

https://github.com/datamallchain

Scope

Payment plan

Disclosure policy

If no errors are found in the contract, the head of the security department may notify the customer about the completion of the audit and publish the report immediately after the completion of the audit.

If errors of medium, high or critical severity were found in the contract, then the head of the Security Department must contact the developer of the smart contract and report any errors found during the audit. The head of the Security Department must not publish the results within 30 days after the completion of the audit and finding errors.

After 30 days from the date of the completion of the audit, the head of the Security Department must publish the results of the audit, including whether or not the client sufficiently remediated the vulnerability and regardless of the severity of the findings and a reaction of the developers of the smart-contract.

Contact information (optional)

vwchen@gmail.com https://dmctech.io/ https://twitter.com/datamallcoin https://t.me/DMC_Foundation https://discord.com/invite/XfvgR3zTMG

Platform

Our contract will be deployed on EOS.

yuriy77k commented 1 year ago

@thi-fogworks please provide a link to the contract that you want to audit to avoid misunderstanding.

thi-fogworks commented 1 year ago

https://github.com/datamallchain/dmchain_contract/tree/main/dmc.contracts/eosio.token/include/eosio.token

https://github.com/datamallchain/dmchain_contract/tree/main/dmc.contracts/eosio.token/src

thi-fogworks commented 1 year ago

Note that both of those are directories with multiple contracts in them. We would like them all audited.

yuriy77k commented 1 year ago

@thi-fogworks the audit fee is 4428 USDT. You may send USDT (ERC20 or BEP20) to: 0x6317c6944bd1cD3932d062cce39d7Fd602119529

The estimated auditing time - is 15 days after payment.

thi-fogworks commented 1 year ago

Can I get a formal invoice? Just for record keeping.

thi-fogworks commented 1 year ago

Made out to: DMCTECH FOUNDATION LTD 3 Fraser Street

05-25 Duo Tower

Singapore 189352

yuriy77k commented 1 year ago

@thi-fogworks to send you invoice we need your tax id number.

ZansticK commented 1 year ago

@thi-fogworks to send you invoice we need your tax id number.

It should be 202204265C

yuriy77k commented 1 year ago

Invoice was sent to email

thi-fogworks commented 1 year ago

Which email address?

From: Yuriy @.> Sent: Wednesday, September 28, 2022 2:04 PM To: EthereumCommonwealth/Auditing @.> Cc: Thi Thumasathit @.>; Mention @.> Subject: Re: [EthereumCommonwealth/Auditing] Need C++ Smart Contract Audited (Issue #671)

Invoice was sent to email

— Reply to this email directly, view it on GitHubhttps://github.com/EthereumCommonwealth/Auditing/issues/671#issuecomment-1261463318, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A26RZNYYLFQQF6TZVKYFG5TWASXHLANCNFSM6AAAAAAQLZL3NY. You are receiving this because you were mentioned.Message ID: @.***>

yuriy77k commented 1 year ago

Which email address?

vwchen@gmail.com

yuriy77k commented 1 year ago

@thi-fogworks did you receive the invoice?

ZansticK commented 1 year ago

Hi there, we already received the invoice, but we still a question：

  1. Does the document also have to be open source? For example, the DMC interface call documentation
yuriy77k commented 1 year ago

Hi there, we already received the invoice, but we still a question：

1. Does the document also have to be open source? For example, the DMC interface call documentation

It's up to you. If you don't want to make it public you can send it privately to my email: yuriy@callistoenterprise.com

yuriy77k commented 1 year ago

@thi-fogworks payment received. Auditing started.

Dexaran commented 1 year ago

I modified the request description to include "Scope" paragraph with contract code folders provided by the requestor:

https://github.com/datamallchain/dmchain_contract/tree/main/dmc.contracts/eosio.token/include/eosio.token

https://github.com/datamallchain/dmchain_contract/tree/main/dmc.contracts/eosio.token/src

As the payment is received I assign "approved" / "eos" status to this request

Dexaran commented 1 year ago

I will start working on this audit request on the next week (30 / 10 / 2022) and it will take approx. 10 days to finish the review.

Dexaran commented 1 year ago

I will most likely complete the review by Wed (9 / 11 / 2022)

Dexaran commented 1 year ago

@yuriy77k Audit report is submitted. Verifying last details.

ZansticK commented 1 year ago

@yuriy77k Audit report is submitted. Verifying last details.

Hello,is the audit completed?

yuriy77k commented 1 year ago

Data Mall Coin (DMC) Security Audit Report

1. Summary

Data Mall Coin (DMC) smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit 4e4070cdef9ed001b2851ed36822b795c1f90934

2.1 Excluded

The business logic of the audited application is outside of the scope of this audit. This is a security audit intended to verify the correctness of the technical part of the system and ensure its fault resistance.

3. Findings

In total, 5 issues were reported, including:

In total, 9 notes were reported, including:

3.1 Indirect logical expression can potentially lead to missing overflow checks

Severity: note

Code snippet

    eosio_assert(price >= 0.0001 && price < std::pow(2, 32), "invaild price");
    eosio_assert(asset.amount > 0, "must bill a positive amount");

    uint64_t price_t = price * std::pow(2, 32);

Description

In this expression, price_t is defined by the value of price multiplied by 2^32, and this can not lead to overflow because the previous assertion price >= 0.0001 && price < std::pow(2, 32) regulates the value of price i.e. it is a positive value less than 2^32.

However, there is no overflow check specifically for price_t and change in L16 can potentially lead to the breach of the price_t logic. This is not the case in the current version of the contract 4e4070cdef9ed001b2851ed36822b795c1f90934, but it can be safer to implement a direct check to avoid any potential problems in the future if the code is updated.

Recommendation

Implement an overflow check specifically for price_t after its assignment in L19.

3.2 Comment typo (Observation / not a security flaw)

Severity: note

Code snippet

Description

It should be "invalid price" / "invalid rate" etc.

3.3 Missing memo length check

Severity: note

Code snippet

Description

Memo length should not exceed 256 characters.

In the current 4e4070cdef9ed001b2851ed36822b795c1f90934 state of the contract, the overflow cannot be directly caused since memo is the only arg that liquidation function accepts but it should be taken into account.

3.4 change_order function accepts name payer and does nothing with it (Unclear)

Severity: note

Code snippet

Description

Is it intentional? Or authorization requirement is missing require_auth(payer); ? Documentation does not provide a sufficient explanation of how this function is intended to work.

3.5 Hardcoded privileged account name

Severity: low

Code snippet

Description

dmcmonitor11 account is hardcoded as a privileged account to call destorypst function. This is not a good practice of having just one strictly hardcoded account with privileges. In case the privileged account will be (1) lost, (2) compromised, (3) it will not have enough resources to perform a transaction on EOSIO mainnet or (4) it will be blacklisted by Block Producers for some reason, the system will require a significant update.

It would be more scalable to introduce a more sophisticated access management system.

Recommendation

    require_auth(payer);
    account_name payer_name1 = N(dmcmonitor11);
    account_name payer_name2 = N(name.x);
    eosio_assert(payer == payer_name1 || payer == payer_name2, "only privileged account can use this function");

3.6 Privileged account dmcmonitor11 does not exist

Severity: low

Code snippet

Description

This privileged account is not yet signed up on EOSIO mainnet. It can be potentially taken by someone before the audited DMC contracts will be deployed.

Recommendation

Sign up for a privileged account first, then update the code so that an already existing account under developers' control would be used as a privileged one.

3.7 Privileged account should have a significant amount of resources

Severity: note

Code snippet

Description

It is possible that this privileged account will be required to perform an action on its own, so it is recommended to stake enough CPU and NET. EOSIO users normally rely on "free CPU/NET" but it can be unavailable in case of ongoing network attacks or depletion of resources on the CPU/NET provider.

Recommendation

Stake CPU and NET on the privileged account.

3.8 There are no "halt" functions implemented

Severity: note

Description

In recent years, most of the hacks have happened to DeFi platforms. Every platform that operates with significant stakes of funds is a big target for hackers. As a result, the platform must be as fault-tolerant as possible, and it must be prepared for worst-case scenarios.

In order to be prepared for such scenarios, the structure of the platform must allow for "emergency stops" that will be enacted should any suspicious activity be detected.

It is possible to remove the bytecode from the contract account completely in case of emergency, thus rendering it "frozen" but it can be better to implement built-in halt functions in case of a system of interconnected contracts.

Recommendation

Implement a "status" contract that will only store a state variable that will allow the platform to operate or render it "frozen".

Let other contracts read the state of the "status" contract.

Implement a security check that will allow to effective freeze all contract functions in all contracts at the same time should the state of the "status" contract change:

eosio_assert(status == "operating", "all functions can be called when _status_ contract is in _operating_ mode");

3.9 There are no cleanup functions in the system

Severity: note

Description

If it is required to update or change data structures in the contract within an update of the contract code, then it will require to empty the existing data structures first. A contract with existing data structures can not be updated or it will result in a breach of the functions that interact with the data structures.

As a result, it may be necessary to implement data cleanup functions in the contract logic.

3.10 require_auth(eos_account) requires authorization from the system contract (Unclear)

Severity: note

Code snippet

Description

eos_account is assigned to eosio which is a system account.

3.11 Memo check requires memo to be less than 512 bytes

Severity: low

Code snippet

Description

Common check is 256 bytes.

3.12 nfttransfer and nfttransferb are not notifying recipients

Severity: low

Code snippet

Description

The NFT transfer function does not notify the RECIPIENT and SENDER of the transaction. Communication model implementation is missing.

Recommendation

Add require_recipient( from ) and require_recipient( to ) to the nfttransfer and nfttransferb functions.

3.13 Only the asset contract (owner) can destroy the record of its asset in the contract table

Severity: owner privileges

Code snippet

Description

Only the asset contract (contract of each token) can cause its symbol to be erased from the table of the exchange contract. It can be a good practice to have another option to erase the token records with a privileged account in order to clean up the exchange contract.

3.14 exissue function does not notify the recipient of the new token if to == foundation

Severity: low

Code snippet

Description

In the case of exissue function call extranfer in subsequent call will notify the recipient of the incoming transaction. However, if to != foundation is false i.e. to and foundation is the same account, then no notification will be created. This can result in foundations balance being greater than it expects as it fails to record new tokens in case foundation is a contract.

4. Security practices

5. Conclusion

The audited smart contract can be deployed. Only low-severity issues were found during the audit.

It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.

ZansticK commented 1 year ago

Data Mall Coin (DMC) Security Audit Report

1. Summary

Data Mall Coin (DMC) smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit 4e4070cdef9ed001b2851ed36822b795c1f90934

2.1 Excluded

The business logic of the audited application is outside of the scope of this audit. This is a security audit intended to verify the correctness of the technical part of the system and ensure its fault resistance.

3. Findings

In total, 5 issues were reported, including:

  • 0 high severity issues.
  • 0 medium severity issues.
  • 5 low-severity issues.

In total, 9 notes were reported, including:

  • 8 notes.
  • 1 owner privilege.

3.1 Indirect logical expression can potentially lead to missing overflow checks

Severity: note

Code snippet

    eosio_assert(price >= 0.0001 && price < std::pow(2, 32), "invaild price");
    eosio_assert(asset.amount > 0, "must bill a positive amount");

    uint64_t price_t = price * std::pow(2, 32);

Description

In this expression, price_t is defined by the value of price multiplied by 2^32, and this can not lead to overflow because the previous assertion price >= 0.0001 && price < std::pow(2, 32) regulates the value of price i.e. it is a positive value less than 2^32.

However, there is no overflow check specifically for price_t and change in L16 can potentially lead to the breach of the price_t logic. This is not the case in the current version of the contract 4e4070cdef9ed001b2851ed36822b795c1f90934, but it can be safer to implement a direct check to avoid any potential problems in the future if the code is updated.

Recommendation

Implement an overflow check specifically for price_t after its assignment in L19.

3.2 Comment typo (Observation / not a security flaw)

Severity: note

Code snippet

Description

It should be "invalid price" / "invalid rate" etc.

3.3 Missing memo length check

Severity: note

Code snippet

Description

Memo length should not exceed 256 characters.

In the current 4e4070cdef9ed001b2851ed36822b795c1f90934 state of the contract, the overflow cannot be directly caused since memo is the only arg that liquidation function accepts but it should be taken into account.

3.4 change_order function accepts name payer and does nothing with it (Unclear)

Severity: note

Code snippet

Description

Is it intentional? Or authorization requirement is missing require_auth(payer); ? Documentation does not provide a sufficient explanation of how this function is intended to work.

3.5 Hardcoded privileged account name

Severity: low

Code snippet

Description

dmcmonitor11 account is hardcoded as a privileged account to call destorypst function. This is not a good practice of having just one strictly hardcoded account with privileges. In case the privileged account will be (1) lost, (2) compromised, (3) it will not have enough resources to perform a transaction on EOSIO mainnet or (4) it will be blacklisted by Block Producers for some reason, the system will require a significant update.

It would be more scalable to introduce a more sophisticated access management system.

Recommendation

  • It is possible to use at least two privileged accounts so that each of them would be allowed to perform the required action.
    require_auth(payer);
    account_name payer_name1 = N(dmcmonitor11);
    account_name payer_name2 = N(name.x);
    eosio_assert(payer == payer_name1 || payer == payer_name2, "only privileged account can use this function");
  • Alternatively, it is possible to store privileged_account_name in a singletone and have one more privileged account that can manage this singletone content, and therefore, it will not be as likely that those two accounts will become compromised at the same time.

3.6 Privileged account dmcmonitor11 does not exist

Severity: low

Code snippet

Description

This privileged account is not yet signed up on EOSIO mainnet. It can be potentially taken by someone before the audited DMC contracts will be deployed.

Recommendation

Sign up for a privileged account first, then update the code so that an already existing account under developers' control would be used as a privileged one.

3.7 Privileged account should have a significant amount of resources

Severity: note

Code snippet

Description

It is possible that this privileged account will be required to perform an action on its own, so it is recommended to stake enough CPU and NET. EOSIO users normally rely on "free CPU/NET" but it can be unavailable in case of ongoing network attacks or depletion of resources on the CPU/NET provider.

Recommendation

Stake CPU and NET on the privileged account.

3.8 There are no "halt" functions implemented

Severity: note

Description

In recent years, most of the hacks have happened to DeFi platforms. Every platform that operates with significant stakes of funds is a big target for hackers. As a result, the platform must be as fault-tolerant as possible, and it must be prepared for worst-case scenarios.

In order to be prepared for such scenarios, the structure of the platform must allow for "emergency stops" that will be enacted should any suspicious activity be detected.

It is possible to remove the bytecode from the contract account completely in case of emergency, thus rendering it "frozen" but it can be better to implement built-in halt functions in case of a system of interconnected contracts.

Recommendation

Implement a "status" contract that will only store a state variable that will allow the platform to operate or render it "frozen".

Let other contracts read the state of the "status" contract.

Implement a security check that will allow to effective freeze all contract functions in all contracts at the same time should the state of the "status" contract change:

eosio_assert(status == "operating", "all functions can be called when _status_ contract is in _operating_ mode");

3.9 There are no cleanup functions in the system

Severity: note

Description

If it is required to update or change data structures in the contract within an update of the contract code, then it will require to empty the existing data structures first. A contract with existing data structures can not be updated or it will result in a breach of the functions that interact with the data structures.

As a result, it may be necessary to implement data cleanup functions in the contract logic.

3.10 require_auth(eos_account) requires authorization from the system contract (Unclear)

Severity: note

Code snippet

Description

eos_account is assigned to eosio which is a system account.

3.11 Memo check requires memo to be less than 512 bytes

Severity: low

Code snippet

Description

Common check is 256 bytes.

3.12 nfttransfer and nfttransferb are not notifying recipients

Severity: low

Code snippet

Description

The NFT transfer function does not notify the RECIPIENT and SENDER of the transaction. Communication model implementation is missing.

Recommendation

Add require_recipient( from ) and require_recipient( to ) to the nfttransfer and nfttransferb functions.

3.13 Only the asset contract (owner) can destroy the record of its asset in the contract table

Severity: owner privileges

Code snippet

Description

Only the asset contract (contract of each token) can cause its symbol to be erased from the table of the exchange contract. It can be a good practice to have another option to erase the token records with a privileged account in order to clean up the exchange contract.

3.14 exissue function does not notify the recipient of the new token if to == foundation

Severity: low

Code snippet

Description

In the case of exissue function call extranfer in subsequent call will notify the recipient of the incoming transaction. However, if to != foundation is false i.e. to and foundation is the same account, then no notification will be created. This can result in foundations balance being greater than it expects as it fails to record new tokens in case foundation is a contract.

4. Security practices

  • [x] Open-source contact.
  • [ ] The contract should pass a bug bounty after the completion of the security audit.
  • [ ] Public testing.
  • [ ] Automated anomaly detection systems. - NOT IMPLEMENTED. A simple anomaly detection algorithm is recommended to be implemented to detect behavior that is atypical compared to normal for this contract. For instance, the contract must halt deposits in case a large amount is being withdrawn in a short period of time until the owner or the community of the contract approves further operations.
  • [ ] Multisig owner account.

5. Conclusion

The audited smart contract can be deployed. Only low-severity issues were found during the audit.

It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract. Hi there! We have revised the listed issues, will you give a new report or do we need further action?

yuriy77k commented 1 year ago

@ZansticK let me know if you want to reaudit your contract after fixing issues.

ZansticK commented 1 year ago

Our code is expected to have another update in mid-December, and we would like to audit it after the update, is that possible?

yuriy77k commented 1 year ago

@ZansticK yes, you'll get a 50% discount on the re-audit fee.

datamallchain commented 1 year ago

3.1 The verification of price range has been done in the above judgment to ensure that no integer overflow will occur, so no fix will be done.

3.2 Spelling will be corrected in next version

3.3 & 3.11 The verification of memo length will be fixed in next version.

3.4 Change_order is an internal method; payer is reserved filed.

3.5 & 3.6 & 3.7 The code is for special business, which has been deleted

3.8 Unnecessary business requirement

3.9 Unusual operation, which will be supportable during data migration

3.10 Necessary business requirement

3.12 the method is unnecessary according to business requirement

3.13 & 3.14 necessary business requirement

Dexaran commented 1 year ago

Any update on this?

thi-fogworks commented 1 year ago

Ie, reaudit?

Get Outlook for iOShttps://aka.ms/o0ukef

From: Dexaran @.> Sent: Tuesday, April 25, 2023 5:10:39 PM To: EthereumCommonwealth/Auditing @.> Cc: Thi Thumasathit @.>; Mention @.> Subject: Re: [EthereumCommonwealth/Auditing] Need C++ Smart Contract Audited (Issue #671)

Any update on this?

— Reply to this email directly, view it on GitHubhttps://github.com/EthereumCommonwealth/Auditing/issues/671#issuecomment-1522485887, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A26RZN4ZOO5TZS2OKDNHFY3XDBDV7ANCNFSM6AAAAAAQLZL3NY. You are receiving this because you were mentioned.Message ID: @.***>