EnduracoinToken smart contracts security audit report performed by chhajershrenik.
Commit: c7b4888537804f7690742bca948af446ff7cc48a
In total, 1 issues were reported, including:
- 1 medium severity issue.
In total, 6 notes were reported, including:
- 6 minor observations.
The functions transfer and transferFrom check the _to address corresponding to the address(this), the address of the contract. But the default value of the _to address would be address zero.
- https://github.com/CallistoSecurity/EnduracoinToken/blob/c7b4888537804f7690742bca948af446ff7cc48a/EnduracoinToken.sol#L49
- https://github.com/CallistoSecurity/EnduracoinToken/blob/c7b4888537804f7690742bca948af446ff7cc48a/EnduracoinToken.sol#L56
Consider replacing address(this) with address(0) to prevent the loss of tokens.
The function burnFrom allows the owner of the contract to burn tokens of a wallet, but the wallet could set the allowance for the owner of the contract to zero or reduce the allowance. This would result in the owner not being able to burn tokens of a wallet and the function would revert.
Function cancelChangeRequest() can be used by any Approver to cancel the voting process for the change request even when the quorum is reached for the change request. For instance if one Approver is opposed to change and the majority of the Approver's are in favor of the change request, then an Approver who is against the change can cancel the change request using the function cancelChangeRequest(). This would lead to re-initializing the voting process for the change request.
Allow Approver to cancel the change request only when the quorum is not reached if the quorum is reached the change should be approved.
The state variable upTime declared does not denote the actual upTime of the contract as the upTime is not recomputed.
Consider implementing a view function to compute the upTime of the contract.
- 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).
- 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.
- Functions not used internally could be marked as external
It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract.
- 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 withdrawn in a short period until the owner or the community of the contract approves further operations.
- Multisig owner account.
- Standard ERC20-related issues. - NOT IMPLEMENTED. It is known that 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 schemekeccak256(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 thenoncethat matches that on the original chain. Example: If you have deployed your main contract with address 0x010101 at your 2021th 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 2021th transaction, and it will receive the same address as your mainnet contract.
3.1. Missing zero address check
It's not an issue.
Zero address check implemented in ERC20.sol contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6d74b913885d729b5c72209aa03c4b68a33c794c/contracts/token/ERC20/ERC20.sol#L228-L235
The recommendation to add
require( _to != address(this))is our standard recommendation for ERC20 tokens to protect users from sending tokens to token contract address (it's a very common mistake).