Skip to content

Instantly share code, notes, and snippets.

@yuriy77k

yuriy77k/ETH_Enduracoin_v6_report.md Secret

Last active November 9, 2023 20:52
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save yuriy77k/0ee453e34c9b0cc4fd7129497d136cbf to your computer and use it in GitHub Desktop.
Save yuriy77k/0ee453e34c9b0cc4fd7129497d136cbf to your computer and use it in GitHub Desktop.
Download ZIP
EnduracoinToken v1.0.5.11052023 Security Audit Report
Raw
ETH_Enduracoin_v6_report.md

EnduracoinToken v1.0.5.11052023 Security Audit Report

1. Summary

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

2. In scope

Commit 017974d036b24e7f97b1bd49cec64215c36db162

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

3. Findings

In total, 2 issues were reported, including:

  • 1 high severity issue.

  • 1 medium severity issue.

  • 0 low severity issues.

In total, 2 notes were reported, including:

  • 1 minor observation.

  • 1 owner privilege.

3.1. Token functionality may be blocked for all users for the voting period

Severity: medium

Description

The functions (transfer, approve, transferFrom, increaseAllowance) of EnduracoinToken have the modifier requiresMultiSig(). This modifier applies to check vote results if pendingChange.addrValueToChange == owner(). It means that any approver who creates newChangeRequest with the parameter addressToUse equal to the owner address will block all tokens functionality until voting is finished.

Adding condition && msg.sender != pendingChange.chgInitiator does not fix this issue, because msg.sender will be a user who call one of those functions, so msg.sender != pendingChange.chgInitiator will be true and does not affect on pendingChange.addrValueToChange == owner().

Also, the modifier requiresMultiSig() should allow to owner to call those functions only if this call was approved. So condition || opposeChange >= MIN_VOTES_REQUIRED should be removed.

Code snipped

Recommendation

Change requiresMultiSig() modifier to this one:

    modifier requiresMultiSig() {
        if(msg.sender == owner())  {
            require(forChange >= MIN_VOTES_REQUIRED, "ENDC: Insufficient approvals.");
        }    
        _;
    }

3.2. Anybody can renounce ownership of EnduracoinValue contract

Severity: high

Description

The condition:

if((pendingChangeType == uint8(ChangeType.renounceOwnership)) && (forChange < MIN_VOTES_REQUIRED)) 
    { invalidInput("Invalid fx call to renounce ownership."); }

does not protect against renounce ownership by anyone, because pendingChangeType == uint8(ChangeType.renounceOwnership will be false in most time.

Code snipped

Recommendation

Change this condition pendingChangeType == uint8(ChangeType.renounceOwnership to pendingChangeType != uint8(ChangeType.renounceOwnership.

3.3. Owner privileges

Severity: owner privileges

Description

50 Billion Enduracoin will be pre-minted to the owner's wallet.

3.4. Multiple minor observation

Severity: minor observation

Description

  1. The modifier requiresMultiSig restricts owner from calling the function until voting is finished, but allows anybody else to call the 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.
  1. The getPendingChangeRequest() is a view function, so does not require onlyApprovers modifier.

  1. Owner must be the final approver in voting for transfer, approve, transferFrom, increaseAllowance. If the owner votes earlier the change request can't be approved and all other approvers should vote against it to be able to create a new request. It creates an opportunity for a frontrunning attack by a malicious approver, where he can flip his vote just before the owner's vote (so the owner will not be the last approver).

  2. The functions transferOwnership() and renounceOwnership() should not be called externally. So better to make internal functions that will be called from this contract. For example:

    // override public functions from Ownable contract
    function transferOwnership(address newOwner) public override {
        revert("Not allowed");
    }

    function transferOwnership(address newOwner) public override {
                revert("Not allowed");
    }

In the EnduracoinValue contract create internal functions and call it from contract instead of transferOwnership() and renounceOwnership()

    // unnecessary checks were removed, because these functions calling from contract after passing all requirements 
    function transferOwnershipInternal(address newOwner) internal {
        removeApprover(owner());
        pendingChangeOfAddress[0] = newOwner;
        if (approverAddresses[newOwner] != true) { setApprovers(pendingChangeOfAddress); }
        _transferOwnership(newOwner);
        emit EnduracoinOwnerTransferred(msg.sender, " changed ENDCValueSmartContract owner to : ", newOwner);
    }

    function renounceOwnershipInternal() internal  {
        _transferOwnership(address(0));
        emit EnduracoinOwnerTransferred(msg.sender, " changed ENDCValueSmartContract owner to : ", address(0));
    }

Do the same for Enduracoin contract:

    // override public functions from Ownable contract
    function transferOwnership(address newOwner) public override {
        revert("Not allowed");
    }

    function transferOwnership(address newOwner) public override {
                revert("Not allowed");
    }

    // Add internal function and remove unnecessary checks
    function transferOwnershipInternal(address newOwner) internal {
        _transferOwnership(newOwner);
        emit EnduracoinOwnerTransferred(msg.sender, "ENDC: Ownership transferred to: ", newOwner);
    }

    function renounceOwnershipInternal() internal {
        _transferOwnership(address(0));
        emit EnduracoinOwnerTransferred(msg.sender, "ENDC: changed ENDCTokenContract owner to : ", address(0));
    }
  1. The function removeApprover() in EnduracoinValue and Enduracoin contract are internal and calls from contract after passing all required checks. So you should remove requiresMultiSig modifier and unnecessary checks. For example:
   function removeApprover(address approverToRemove) internal {
        emit EnduracoinOwnerTransferred(msg.sender, "ENDCV: Ownership transferred to: ", approverToRemove);
        require(approverAddresses[approverToRemove], "INFO: ENDCV - The address entered is not in the approvers list.");
        _removeApprover(approverToRemove);
    }
  1. The modifier requiresMultiSig() has a check (msg.sender == owner() && approverAddresses[msg.sender]). The condition approverAddresses[msg.sender] will always be True if the msg.sender is the owner() address. Therefore the condition approverAddresses[msg.sender] can safely be removed.
  1. Change request types setBaseValueAdjustment, and setDailyValueGainAdjustment are unused in the contracts and can be removed.
  1. Condition (pendingChangeType != uint8(ChangeType.renounceOwnership in the function removeApprover always be True, as the contract cannot access the function removeApprover() for the change type ChangeType.renounceOwnership.
  1. When a pending change is executed in EnduracoinValue contract the executionResult string is set to "Change Opposed!", but even if the change was successfully executed the event ChangeRequestExecutedEvent would be emitted with the opposed string instead of a success string.

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 can't be deployed. Pointed issues should be fixed before deployment.

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. Previous audit reports

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