Auditing icon indicating copy to clipboard operation
Auditing copied to clipboard

Need C++ Smart Contract Audited

Open thi-fogworks opened this issue 3 years ago • 26 comments

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

  • 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

Payment plan

  • [ ] Standard (standard projects such as ICOs, ERC20, ERC223, ERC721, ERC721A tokens and NFTs fall in this category - 2 auditors. $500 + $1 per line of code / minimum $1000)

  • [X] Advanced (non-standard projects requiring more careful review - 3 auditors. $1000 + $1.25 per line / minimum $2000)

  • [ ] Corporate (projects that require post-launch support - 3 auditors - Callisto Team will handle the bugbounty and provide a 1 month long period of technical support regarding the necessary security enhancement procedures and track/highlight any issues and potential threats - this plan allows to further apply for Callisto DAPP Insurance program)

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)

[email protected] 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.

thi-fogworks avatar Sep 13 '22 20:09 thi-fogworks

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

yuriy77k avatar Sep 14 '22 19:09 yuriy77k

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 avatar Sep 14 '22 20:09 thi-fogworks

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

thi-fogworks avatar Sep 14 '22 20:09 thi-fogworks

@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.

yuriy77k avatar Sep 16 '22 15:09 yuriy77k

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

thi-fogworks avatar Sep 16 '22 16:09 thi-fogworks

Made out to: DMCTECH FOUNDATION LTD 3 Fraser Street #05-25 Duo Tower Singapore 189352

thi-fogworks avatar Sep 16 '22 17:09 thi-fogworks

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

yuriy77k avatar Sep 20 '22 12:09 yuriy77k

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

It should be 202204265C

ZansticK avatar Sep 21 '22 06:09 ZansticK

Invoice was sent to email

yuriy77k avatar Sep 28 '22 21:09 yuriy77k

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: @.***>

thi-fogworks avatar Sep 28 '22 21:09 thi-fogworks

Which email address?

[email protected]

yuriy77k avatar Sep 29 '22 10:09 yuriy77k

@thi-fogworks did you receive the invoice?

yuriy77k avatar Sep 30 '22 09:09 yuriy77k

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

ZansticK avatar Oct 08 '22 02:10 ZansticK

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: [email protected]

yuriy77k avatar Oct 12 '22 11:10 yuriy77k

@thi-fogworks payment received. Auditing started.

yuriy77k avatar Oct 19 '22 15:10 yuriy77k

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 avatar Oct 27 '22 16:10 Dexaran

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 avatar Oct 27 '22 16:10 Dexaran

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

Dexaran avatar Nov 07 '22 20:11 Dexaran

@yuriy77k Audit report is submitted. Verifying last details.

Dexaran avatar Nov 10 '22 19:11 Dexaran

@yuriy77k Audit report is submitted. Verifying last details.

Hello,is the audit completed?

ZansticK avatar Nov 16 '22 01:11 ZansticK

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L16-L19
    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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L16

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L205

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L312

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L479-L483

Description

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

3.3 Missing memo length check

Severity: note

Code snippet

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L346

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L516

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L660

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L10

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L348

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L478

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L518

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L104

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L143

Description

Common check is 256 bytes.

3.12 nfttransfer and nfttransferb are not notifying recipients

Severity: low

Code snippet

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L101-L138

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L140-L185

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/smart_extend.cpp#L10-L42

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/smart_token.cpp#L58-L60

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.

yuriy77k avatar Nov 20 '22 00:11 yuriy77k

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L16-L19
    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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L16
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L205
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L312
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L479-L483

Description

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

3.3 Missing memo length check

Severity: note

Code snippet

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L346
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L516
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L660

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L10

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc_deliver.cpp#L298

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L348
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L478
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/dmc.cpp#L518

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L104
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L143

Description

Common check is 256 bytes.

3.12 nfttransfer and nfttransferb are not notifying recipients

Severity: low

Code snippet

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L101-L138
  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/nft.cpp#L140-L185

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/smart_extend.cpp#L10-L42

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

  • https://github.com/datamallchain/dmchain_contract/blob/4e4070cdef9ed001b2851ed36822b795c1f90934/dmc.contracts/eosio.token/src/smart_token.cpp#L58-L60

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?

ZansticK avatar Nov 25 '22 06:11 ZansticK

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

yuriy77k avatar Nov 26 '22 15:11 yuriy77k

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

ZansticK avatar Dec 01 '22 06:12 ZansticK

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

yuriy77k avatar Dec 01 '22 19:12 yuriy77k

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

datamallchain avatar Dec 08 '22 07:12 datamallchain

Any update on this?

Dexaran avatar Apr 25 '23 22:04 Dexaran

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: @.***>

thi-fogworks avatar Apr 25 '23 22:04 thi-fogworks