search

CallistoSecurity / Smart-contract-auditing

This is a working repo of @EthereumCommonwealth audits. We performed more than 400 security audits since 2018. Not even a single contract was hacked after our auditors approved the code. Accepting audit requests here.
https://audits.callisto.network/
2 stars 2 forks source link

Enduracoin Request #5

Closed Enduracoin closed 1 year ago

Enduracoin commented 1 year ago

Audit request

... Briefly describe your smart-contract and its main purposes here ...

Hopefully this will be a simple one for you. This request consists of two primary files:

1.) EnduracoinToken.sol which is our ERC20 token contract. It is built upon the OpenZeppelin 4.8.3 contracts release and Solidity compiler version 0.8.20. It's fairly simple and self-explanatory with no significant deviations from the OZ 4.8.3 version contracts. We have overridden the burn, burnFrom, transferOwnership and renounceOwnership methods to apply onlyOwner execution permissions. A "safetyLock" has been applied as an extra step for our internal team processes. We've also implemented the pause and unpause methods.

and

2.) EnduracoinValue.sol which is a ERC20 contract that is used to convey a value of our product via ongoing daily calculations. This contract provides the ability for the conveyed value to be modified via the setDailyValueGainAdjustment and setBaseValueAdjustment methods with corresponding events for each. The conveyed value is presented as a string value representing a decimal number for easy interpretation. This contract also implements a "safetyLock" which has been applied as an extra step for our internal team processes. This contract does not receive, hold nor transfer any tokens.

The OpenZeppelin 4.8.3 contracts that we've used are included in the @openzeppelin/contracts@4.8.3 locations in our repo and can be accessed via the following URL: https://github.com/Enduracoin/EnduracoinToken/tree/main/%40openzeppelin/contracts%404.8.3

Source code

The source repo we've created for this audit is located at https://github.com/Enduracoin/EnduracoinToken It is currently in a private repo so please email us the github username or email address for the auditors to receive access that we may grant that access and add as a collaborator for this repo. You can email us at troy@enduracoin.org.

Payment plan

... Write [x] at the checkbox of the payment plan that suits your needs ...

Disclosure policy

... Do you want us to publish the report as it is or to notify you privately in case of finding critical mistakes? ...

If no findings you may publish the report. If there are findings, please notify us privately that we may remedy and resubmit to pass before publishing. Thank you.

... provide your conditions for publishing the report or leave only standard disclosure policy link ...

If no findings you may publish the report. If there are findings, please notify us privately that we may remedy and resubmit to pass before publishing. Thank you.

Standard disclosure policy.

Contact information (optional)

... Provide information to contact you or the smart contract-developer in case high severity issues will be found ...

Please send all correspondence of results, findings and recommendations to troy@enduracoin.org

... Provide information about the media resources of the project you want us to audit (website/ twitter account/ reddit/ telegram channel/ etc.) ...

I'm not quite sure what this is asking and apologies if this isn't what you're looking for. Feel free to contact us for clarification if this answer isn't what you're asking...

Assuming there are no findings you may communicate the report to whatever media resources you would commonly publish to. However, if there are findings, please notify us privately that we may remedy and resubmit to pass before publishing. We will disclose our results upon successful notification via our website, enduracoin.org and via our Twitter account, @Enduracoin, and via Etherscan.io. There is no direct tie to our code and any media resource operations.

Platform

... In which network will your contract be deployed? (EOS/TRX/ETC/ETH/CLO/UBQ/something else ) ...

ETH - Ethereum Mainnet

Thank you.

Enduracoin commented 1 year ago

Hello @yuriy77k,

I have submitted the 1200 USDT payment to 0x6317c6944bd1cD3932d062cce39d7Fd602119529 as you've requested.

I have also recreated the audit request at the new URL provided: https://github.com/CallistoSecurity/Smart-contract-auditing/issues/

It is request #5.

Enduracoin commented 1 year ago

Hello @yuriy77k and team,

I received an email from a Kunal Mishra with Github username of Kunalmishra1999. The subject line of the email reads, "Enduracoin audit contest". However, there is no other information in the body of the email, no instructions, etc.

Is Kunal Mishra a member of your audit team and a part of the Enduracoin audit? If so, are there any actions that I need to take?

Sincerely, @Enduracoin

yuriy77k commented 1 year ago

Hello @Enduracoin Yes, he is our new junior auditor. He was not assigned to the Enduracoin audit, so you don't need to do any action. Sorry for inconvenience

yuriy77k commented 1 year ago

@Enduracoin contract has a medium severity issue, and the report was sent to your email.

Enduracoin commented 1 year ago

Hello @yuriy77k,

I apologize, as I believe there is some misunderstanding as to how the value contract operates. I will have the documentation updated to explain and clarify it's use in a couple of days.

Will this constitute a new audit request or is there a grace period for us to remedy the severity issue found before the audit is deemed complete?

yuriy77k commented 1 year ago

Hello @Enduracoin,

If you fix pointed issues, the re-audit will be for free. But if you add new functions, you should pay for re-audit 50% of the initial payment.

Enduracoin commented 1 year ago

Hello @yuriy77k and team,

I have uploaded updated code with respect to your findings. Some code was removed from the token contract and some code was added to the value contract. Some of the additions were separated into 2 additional files. Readme info has been added as well. Shall I make the re-audit payment to the same address as before (0x6317c6944bd1cD3932d062cce39d7Fd602119529) in USDT?

chhajershrenik commented 1 year ago

@Enduracoin Please wait @yuriy77k will reach out with an update.

Enduracoin commented 1 year ago

Ok, thank you.

yuriy77k commented 1 year ago

Hello @Enduracoin The re-audit cost is 600 USDT Please, make re-audit payment to the same address as before (0x6317c6944bd1cD3932d062cce39d7Fd602119529)

Estimate auditing time: 5 days after payment.

Enduracoin commented 1 year ago

Hello @yuriy77k,

I have sent the payment to 0x6317c6944bd1cD3932d062cce39d7Fd602119529 and the transaction has completed. Thanks.

yuriy77k commented 1 year ago

@Enduracoin money received. Audit started.

yuriy77k commented 1 year ago

Medium-severity issues were found. The report was sent to the developer.

Enduracoin commented 1 year ago

Thank you. Looks like we've additional work. We'll get back shortly.

Enduracoin commented 1 year ago

Source files have been updated and the 3 findings have been addressed. Let me know of next steps.

Regarding the security practices, the developer has also asked me to pass along 3 questions for some guidance regarding the following minor issues that were found during the audit and then I have one follow up regarding the post audit efforts.

Developer questions: 1.) Crosschain address collisions. The developer has asked if you can direct him to online videos explaining this and how to implement it as it's not something he has encountered before?

2.) Automated anomaly detection - Can you provide docs or videos on best practices for implementing this and the default values (i.e. what trigger points should be set and how much withdrawal is too much)? There are instances where individuals will legitimately want to move large amounts of the token. We don't want to hinder these and would like to have zero manual effort if possible but want to maintain security as well.

My question: Regarding the bug bounty and public testing once the audit is finally complete... I understand that Callisto can facilitate the bug bounty and public testing? How does that work, what is the time duration for the bounties and what would the estimated expense look like for this?

Thank you, @Enduracoin.

Dexaran commented 1 year ago

1.) Crosschain address collisions. The developer has asked if you can direct him to online videos explaining this and how to implement it as it's not something he has encountered before?

Unfortunately we can't provide any videos right now. We are not in a cooperation with any content makers and we don't make videos ourselves - but we can provide an instruction in a written text. We can write it here in the issue comment thread or you can suggest any other way of communication that suits you.

2.) Automated anomaly detection - Can you provide docs or videos on best practices for implementing this and the default values (i.e. what trigger points should be set and how much withdrawal is too much)? There are instances where individuals will legitimately want to move large amounts of the token. We don't want to hinder these and would like to have zero manual effort if possible but want to maintain security as well.

Well, anomaly detection isn't something new https://aws.amazon.com/what-is/anomaly-detection/

It is very project specific thing and we recommend contract developers to examine their projects and identify what kinds of activity are considered "normal" and what activities are not. This is not a widely adopted practice in crypto (probably thats why we witness a huge number of hacks) so I doubt that there are any ready-to-go solutions.

You can:

Regarding the bug bounty and public testing once the audit is finally complete... I understand that Callisto can facilitate the bug bounty and public testing?

We can deploy a prototype on CLO chain (it is very cheap) or any testnet (it is free).

Normally a bugbounty can run for few weeks. We can help you with the announcement and spread it over our social media as well.

Enduracoin commented 1 year ago

@Dexaran, Thank you for your response. Yes, please provide detailed crosschain address collision remediation instructions here in this comment thread. As mentioned, none of us have dealt with this before, so the more details the better. I do appreciate it.

We will look into the automatic anomaly detection link that you sent and determine if there is a significant enough impact to our project to pursue.

I plan to work on multisig transaction approver methods for our token contract throughout the remainder of the week. We should be good to go thereafter with continuing the audit. I've implemented something similar for our value contract. I will respond once the token contract is added.

Finally, does Callisto provide development services that could assist with the remainder of our efforts? Especially with these newer vulnerability issues that aren't so well known.

Thanks again!

Dexaran commented 1 year ago

Finally, does Callisto provide development services that could assist with the remainder of our efforts? Especially with these newer vulnerability issues that aren't so well known.

cc @yuriy77k

Dexaran commented 1 year ago

Thank you for your response. Yes, please provide detailed crosschain address collision remediation instructions here in this comment thread. As mentioned, none of us have dealt with this before, so the more details the better. I do appreciate it.

Crosschain address collision can not directly affect your funds but it can affect your users. Take a look at how Ethereum contract address is assigned: https://ethereum.org/en/developers/docs/accounts/#:~:text=The%20contract%20address%20is%20usually,(the%20%E2%80%9Cnonce%E2%80%9D).

The contract address is generated from the creator address and nonce - the number of the transaction sent from this address.

However, Ethereum is not the only EVM-compatible blockchain nowadays so this contract address will be valid for all EVM-compatible chains. Which means - a user can send a transaction by mistake on some other chain (for example Ethereum CLassic).

This happened to Golem developers earlier https://www.reddit.com/r/GolemProject/comments/71lrad/request_for_golem_developers_comments_58000_lost/

By default a contract can not accept funds and if a user will send something to your contract by mistake - the transaction will get reverted. But your contract will be deployed at one chain (even though its address will be automatically valid for all chains) and there will be no contract on other chains at the same address - which means if users send anything there by mistake it can not be reverted.

So I recommend you to take the address that you used to deploy the contract on Ethereum and use it to create a "mock contract" on the most popular EVM-compatible chains to prevent users from losing funds that they may accidentally send to your address with a wrong chain selected.

yuriy77k commented 1 year ago

@Enduracoin Since are many changes in the contracts you should pay 300 USDT re-audit fee. Please, transfer USDT to 0x6317c6944bd1cD3932d062cce39d7Fd602119529 (ETH and BSC chains)

Enduracoin commented 1 year ago

The latest code files have been uploaded. Detailed documentation has been added in each file and a multisig type implementation has been added as well. To accommodate this, much of the code was consolidated in the ChangeRequests contract and it is used by both the token and value contracts. These files are now ready for your review. I have also sent the 300 USDT to the wallet you specified. Transaction confirmation status shows complete. Thank you.

Enduracoin commented 1 year ago

Hello @yuriy77k and team. Have you an update for me regarding this request?
I've included our forge test script that we use and have added it to this repository. The rest of our testing for the value contract has been via remix as forge has bugs that are currently being worked. However, our contracts have proven to work fine via remix along with test chains. Please advise as to timeframe. Thank you, @Enduracoin.

yuriy77k commented 1 year ago

Hello @Enduracoin The final report will be ready in 2 days.

Enduracoin commented 1 year ago

Thank you @yuriy77k. I have added documentation to the repository as well. Once this is complete and the audit is good, I'd like to proceed with Callisto facilitating the public testing and bug bounty. @Dexaran mentioned that "We (Callisto) can deploy a prototype on CLO chain (it is very cheap) or any testnet (it is free)". Please let me know what you need to proceed with this.

yuriy77k commented 1 year ago

Hello @Enduracoin

High and medium severity issues were found in your contract. The report was sent by email.

Enduracoin commented 1 year ago

Hello @yuriy77k

We are working to fix the issues and will resubmit for audit.

Thank you and Sincerely,
@Enduracoin

Enduracoin commented 1 year ago

Hello @yuriy77k

We have updated our contracts and uploaded. You may continue with the audit. Not many changes were made other than those your team recommended in the previous round so it should be fairly simple for your team. Any chance this will be reviewed by end of weekend? Thank you, @Enduracoin

yuriy77k commented 1 year ago

@Enduracoin Yes, we will review it by the end of this week.

yuriy77k commented 1 year ago

EnduracoinToken v4 Security Audit Report

1. Summary

EnduracoinToken smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit 2ac940bfee90dba11d0b5499ae3e16d988b47ec6

3. Findings

In total, 0 issues were reported, including:

In total, 3 notes were reported, including:

3.1. Owner privileges

Severity: owner privileges

Description

  1. 50 Billion Enduracoin will be pre-minted to the owner's wallet. If tokens are burnt, the owner has the right to mint new tokens up to 50 Billion in total supply.
  2. The majority of approvers can set any value in the EnduracoinValue contract. So the getCurrentValue in the EnduracoinValue contract does not get a real market value of Enduracoin in a decentralized way.

3.2. Multiple minor observation

Severity: minor observation

Description

  1. The modifier requiresMultiSig restrict owner to call function until voting is finished, but allow anybody else to call function without restriction. Therefore, in the context of the contract EnduracoinValue it does not make any sense and can be removed or replaced by the modifier onlyApprovers.

  2. The getPendingChangeRequest() is a view function, so does not require onlyApprovers modifier.

  3. Owner can't create change request and must be the last approver in voting. If owner vote earlier the chenge reqest can't be approved and all others aprovver should vote against it to be able create new request. It's not a security issue, but may cause inconvinient.

  4. Redundant require statement in the modifier requiresMultiSig, the require statement is unnecessary as the following check is already being performed in the function voteForChangeRequest().

4. Security practices

5. Conclusion

The audited smart contract can be deployed. Users should pay attention to the contract owner's rights.

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.

6. Revealing audit reports

6.1. Previous audit reports

Enduracoin commented 1 year ago

Thank you Yuriy and team. We have a change to make to the code and will submit a new request once ready.

@Enduracoin

On Sep 18, 2023, at 1:46 PM, Yuriy @.***> wrote:



Closed #5https://github.com/CallistoSecurity/Smart-contract-auditing/issues/5 as completed.

— Reply to this email directly, view it on GitHubhttps://github.com/CallistoSecurity/Smart-contract-auditing/issues/5#event-10401746125, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BAF34P4SO3MV6JJGVBONXNTX3CJIJANCNFSM6AAAAAAY3IFTUM. You are receiving this because you were mentioned.Message ID: @.***>

yuriy77k commented 1 year ago

@Enduracoin your contract can be used in the current version. Minor observations aren't security issues.