Skip to content

Instantly share code, notes, and snippets.

@yuriy77k

yuriy77k/ETH_Enduracoin_v3_report.md Secret

Created August 23, 2023 21:01
Show Gist options
  • Save yuriy77k/b84d523240684ca6c4e1d06fdad525c7 to your computer and use it in GitHub Desktop.
Save yuriy77k/b84d523240684ca6c4e1d06fdad525c7 to your computer and use it in GitHub Desktop.
Download ZIP
EnduracoinToken v3 Security Audit Report
Raw
ETH_Enduracoin_v3_report.md

EnduracoinToken v3 Security Audit Report

1. Summary

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

2. In scope

Commit 410ae3c9773815a225459edfc37783a013769643

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

3. Findings

In total, 3 issues were reported, including:

  • 1 high severity issue.

  • 1 medium severity issue.

  • 1 low severity issue.

In total, 3 notes were reported, including:

  • 1 minor observation.

  • 2 owner privileges.

3.1. Anybody can transferOwnership or renounceOwnership

Severity: high

Description

  1. The modifier requiresMultiSig checks that voting is finished only if msg.sender == owner() or pendingChange.addrValueToChange == owner() otherwise function executed independently of voting status.
  2. Functions transferOwnership and renounceOwnership require the pendingChangeType should have an appropriate value (ChangeType.transferOwnership or ChangeType.renounceOwnership accordingly) and do not check the voting result.

Therefore, when a new change request to transferOwnership or renounceOwnership is created, anybody will be able to transfer ownership to any address or renounce ownership.

Code snippet

Recommendation

Add into functions transferOwnership, renounceOwnership requirements:

        require(forChange >= MIN_VOTES_REQUIRED, "Not approved");

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

Severity: medium

Description

The function cleanUpChangeRequests() is called when voting is complete. This function will remove all approvers and delete historicalListOfApprovers if pendingChangeType == ChangeType.renounceOwnership, independently of the voting result. So if "renounceOwnership" request is declined all approvers will be deleted anyway.

Code snipped

Recommendation

Add to if condition check of the voting result:

        if(pendingChangeType == uint8(ChangeType.renounceOwnership) && forChange >= MIN_VOTES_REQUIRED ){

Recommendation

3.3. The new owner will not be added to historicalListOfApprovers

Severity: low

Description

In the function execChangeRequest() in case of pendingChangeType == uint8(ChangeType.transferOwnership) new owner's address will be added to the approver list by function _setApprover(), but this function doesn't add the approver address to historicalListOfApprovers (it was implemented in function setApprovers()).

Code snippet

Recommendation

Move command which adds new approver to historicalListOfApprovers from setApprovers() to _setApprover().

    /**
     * @dev `setApprovers`(`address`[`MAX_APPROVERS`] memory `newApprovers`) adds approver addresses from 
     * the array of `newApprovers` to the list of approvers. Internal function without access restriction.  */
    function setApprovers(address[MAX_APPROVERS] memory newApprovers) internal virtual {
        require(newApprovers.length < MAX_APPROVERS + 1, "Attempted to add too many approvers.");
        for (uint i = 0; i < newApprovers.length; i++) {
            if(newApprovers[i] != address(0)){ 
                _setApprover(newApprovers[i]); 
            }
        }
    }
    
    /**
     * @dev `_setApprover`(address `newApprover`) adds an address of 'newApprover' to the list of approvers.
     * Internal function without access restriction.  */
    function _setApprover(address newApprover) internal {
        require(!approverAddresses[newApprover], "INFO: The approver address is already on the approvers list.");
        require(numberOfApprovers < MAX_APPROVERS, "ERROR: The maximum approver limit has been reached.");
        historicalListOfApprovers.push(newApprover); 
        approverAddresses[newApprover] = true;
        numberOfApprovers++;
        emit ApproverAdded(newApprover);
    }

3.4. 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.5. Multiple minor observation

Severity: minor observation

Description

  1. In the function setApprovers() don't need to check newApprovers.length because newApprovers is a fixed-size array.
  1. The getPendingChangeRequest() is a view function, so does not require onlyApprovers modifier.
  1. The function voteForChangeRequest() has onlyApprovers modifier, so require(approverAddresses[msg.sender] == true) is a double-check and can be removed.
  1. The validateApproverExists() is a view function, so does not require onlyApprovers modifier.
  1. If MIN_VOTES_REQUIRED will be changed, the function voteForChangeRequest will use 4 required votes anyway. Recommend replacing "magic number" 4 with MIN_VOTES_REQUIRED.
  1. Overriding removeApprover function in Enduracoin contract is not necessary and can be removed. All checking was made in other places:
    1. This function is internal and called only from function execChangeRequest() if forChange == MIN_VOTES_REQUIRED.
    2. Checking of removed address was in function newChangeRequest.
    3. Checking of numberOfApprovers was in function newChangeRequest as well.

The same is applicable for removeApprover in EnduracoinValue contract.

  1. Modifier requiresApprovals() should allow calling functions only if a majority voted for. || opposeChange >= MIN_VOTES_REQUIRED should be removed.
  1. Multiple imports of the ManageApprovers contract.

The contract EnduracoinValue inherits the contract's ManageApprovers and ChangeRequests, the import for the contract ManageApprovers is redundant as the contract is already inherited by the contract ChangeRequests.

4. Security practices

  • 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 if a large amount is being withdrawn quickly until the owner or the project community approves further operations.
  • Multisig owner account.
  • Standard ERC20-related issues. - NOT IMPLEMENTED. Every contract can potentially receive an unintended ERC20-token deposit without the ability to reject it, even if the contract is not intended to receive or hold tokens. As a result, it is recommended to implement a function that will allow extracting any arbitrary number of tokens from the contract.
  • Crosschain address collisions. ETH, ETC, CLO, etc. It is possible that a transaction can be sent to the address of your contract at another chain (as a result of a user mistake or some software fault). It is recommended that you deploy a "mock contract" that would allow you to withdraw any tokens from that address or prevent any funds deposits. Note that you can reject transactions of native tokens deposited, but you can not reject the deposits of ERC20 tokens. You can use this source code as a mock contract: extractor contract source code. The address of a new contract deployed using CREATE (0xf0) opcode is assigned following this scheme keccak256(rlp([sender, nonce])). Therefore you need to use the same address that was originally used at the main chain to deploy the mock contract at a transaction with the nonce that matches that on the original chain. Example: If you have deployed your main contract with address 0x010101 at your 2021st transaction, then you need to increase your nonce of 0x010101 address to 2020 at the chain where your mock contract will be deployed. Then you can deploy your mock contract with your 2021st transaction, and it will receive the same address as your mainnet contract.

5. Conclusion

The audited smart contract must not be deployed. Reported issues must be fixed before the usage of this contract.

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 report

@chhajershrenik
Copy link

Issue 3.1. Is invalid as the following check forChange >= MIN_VOTES_REQUIRED || opposeChange >= MIN_VOTES_REQUIRED would not pass. Also if the voting does takes place and the quorum reaches in favor of opposeChange the execChangeRequest() would be executed from the function voteForChangeRequest() and then the cleanUpChangeRequests() would be processed making the function inaccessible to public as the state pendingChange.addrValueToChange is cleared in cleanUpChangeRequests().

@yuriy77k
Copy link
Author

yuriy77k commented Sep 4, 2023 •
edited
Loading

@chhajershrenik
The require of 'forChange >= MIN_VOTES_REQUIRED || opposeChange >= MIN_VOTES_REQUIRED' applies only if 'msg.sender == owner()' or 'pendingChange.addrValueToChange == owner()'
In other cases that requirements are ignored.
So anybody, except owner, can call functions with 'requiresMultiSig' modifier.

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