Skip to content

Instantly share code, notes, and snippets.

@greywolf12

greywolf12/Enduracoin_reaudit_report.md Secret

Last active July 9, 2023 17:10
Show Gist options
  • Save greywolf12/7cb099377b7f997b31108fe982c97c33 to your computer and use it in GitHub Desktop.
Save greywolf12/7cb099377b7f997b31108fe982c97c33 to your computer and use it in GitHub Desktop.
Download ZIP
Enduracoin Security re-audit Report
Raw
Enduracoin_reaudit_report.md

Enduracoin Security re-audit Report

1. Summary

Enduracoin smart contract security audit report performed by GreyWolf

2. In scope

Commit c7b4888537804f7690742bca948af446ff7cc48a

  • EnduracoinToken.sol
  • EnduracoinValue.sol
  • ManageApprovers.sol
  • StringHelpers.sol

3. Findings

In total, 3 issues were reported, including:

  • 0 high severity issues.

  • 2 medium severity issues.

  • 1 low severity issue.

In total, 11 notes were reported, including:

  • 9 minor observation.

  • 2 owner privileges.

3.1. Any approver can cancel change request

Severity: medium

Description

Any approver can cancel change request even if majority voted for it. Also if one approver wallet was compromised he can cancel any request (include request to remove compromised approver) making voting useless.

Code snipped

Recommendation

Since change request can be removed if majority voted against it, better to allow voter to change his vote rather than cancel request.

3.2. All approvers will be removed even if "renounceOwnership" request will be declined

Severity: medium

Description

In case of majority voted against change request the function cleanUpChangeRequests will be called with parameter check = 0. But in this function there is not checks if request was approved or decline. In any case all approvers and dailyValueGain, baseValue will be deleted, making contract useless.

Code snipped

Recommendation

Make parameter check = 0 if change request is declined and check = 1 if request is approved. Remove all approvers only if check == 1.

        if(check == 1 && keccak256(bytes(pendingChangeType)) == keccak256(bytes("renounceOwnership"))){

3.3. 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.4. Owner can be removed from Approver list

Severity: low

Description

In the function removeApprover there is requirement which don't allow to remove owner from approver list. But if ownership was transferred to the new owner using function transferOwnership, the new owner may not be added to approver list and is possible to remove all approvers from the list.

Recommendation

Remove the old owner and add the new owner to approver list while transferOwnership.

3.5. Multiple minor observations

Severity: minor observation

Description

  1. Since ERC20 functions transfer and transferFrom don't trigger any callback function, therefore the reentrance attack is not possible. Recommended to remove from the Enduracoin contract the ReentrancyGuard and nonReentrant modifier to to reduce transactions cost.

  2. The array keyValues in the contract ManageApprovers is not used and can be removed.

  3. The contract EnduracoinValue derived from ManageApprovers, so is not necessary to specify ManageApprovers. when you access to the parent contract. For example here: https://github.com/CallistoSecurity/EnduracoinToken/blob/c7b4888537804f7690742bca948af446ff7cc48a/EnduracoinValue.sol#L48 and in many other places.

  4. When error message in require statement is a single string, does not need to use string.concat

  1. The modifier requiresApprovals already checks that msg.sender is approver, therefore onlyApprovers modifier can be removed from this line:

  1. The variables inFavorOfChange and opposedToChange are used just to add new value to arrayOfApprovals. To reduce transaction gas cost better to use local memory variables instead of global.

  2. The variable currentValue is private but is not use (it only assign) in contract and can't be read (there isn't getter function for it). Recommend to remove this variable.

  3. The variable upTime is private (there isn't getter function for it) and unused. Recommend to remove this variable.

3.6. Follow good coding practice

Severity: minor observation

Description

  1. Missing docstrings

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

  1. Missing test suite

The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure that the contract functions and behaves as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment