Skip to content

Instantly share code, notes, and snippets.

@chhajershrenik

chhajershrenik/BSC_BRICSChainToken_audit_report.md Secret

Last active September 25, 2023 14:47
Show Gist options
  • Save chhajershrenik/e194dc3d80be863911a2697c3f9fce4f to your computer and use it in GitHub Desktop.
Save chhajershrenik/e194dc3d80be863911a2697c3f9fce4f to your computer and use it in GitHub Desktop.
Download ZIP
Binance smart chain (BSC) BRICSChainToken Smart Contract Security Audit Report.
Raw
BSC_BRICSChainToken_audit_report.md

Binance smart chain (BSC) BRICSChainToken Smart Contract Security Audit Report.

1. Summary

BRICSChainToken smart contracts security audit report performed by chhajershrenik.

2. In scope

Commit: c203cc42f2c683269ad88249a658e996943cc266

3. Findings

In total, 4 issues were reported, including:

  • 1 medium severity issues.
  • 3 low severity issues.

In total, 19 notes were reported, including:

  • 12 minor observations.
  • 7 owner privileges.

3.1. Missing zero address checks.

Severity: low

Description

The function deprecate() does not check if the _upgradedAddress is not address(0), if the _upgradedAddress is set to address(0) by default or by mistake this could lead to unexpected reverts to the contracts functionalities. Additionally, consider adding a public flag isUpgradedToken to the contract at the _upgradedAddress address.

Functions transfer() and transferFrom() allow the destination address (_to) to address zero, which would lead to a loss of wallet token if tokens are sent to address(0) by default or by mistake.

Code Snippet

Recommendation

Consider adding a zero address check to the deprecate(), transfer() and transferFrom() functions.

3.2. Token approvals can be changed when the contract state is paused.

Severity: low

Description

The function approve() can used to modify token approvals for a spender even when the contract state is paused.

Code Snippet

Recommendation

Consider adding the modifier whenNotPaused to the function approve().

3.3. Incorrect short address check.

Severity: medium

Description

The short address attack check via the onlyPayloadSize modifier is not considered a correct mitigation for the attack and is even potentially harmful when extending contracts.

If the functions transfer() and transferFrom() are used by a subcontract's function with fewer arguments the modifier onlyPayloadSize with the short address check will fail. This was fixed in Solidity 0.5.0: ethereum/solidity#4224.

Code Snippet

Recommendation

Consider removing the modifier onlyPayloadSize and upgrading the contract version.

3.4. Functions transfer() and transferFrom() do not return a boolean flag.

Severity: low

Description

Functions transfer() and transferFrom() do not return the boolean flag true for successful operations, if any contract interacted with the BRICSChainToken token using the standard ERC20 interface, the function would return false by default even for successful operations.

Code Snippet

Recommendation

Consider returning a boolean flag for true or false the functions transfer() and transferFrom() for successful or reverted transactions respectively.

3.5. Owner privileges

Severity: owner privileges

Description

  1. Functions pause() and unpause() allow the owner to change the active state of the contract, pausing the state of the contract disables the transfer of tokens for all users.
  2. Functions addBlackList() and removeBlackList() allows the owner to add or remove a wallet to/from the blacklist the blacklisted user cannot transfer tokens.
  3. Function destroyBlackFunds() allows the owner to destroy all funds of a blacklisted wallet.
  4. Function deprecate() allows the owner of the contract to upgrade and implement the contract's existing logic.
  5. Functions issue() and redeem() allows the owner to Issue or Redeem the token from the owner's wallet.
  6. Function mint() and mintTo() allows the owner to mint an arbitrary amount of tokens for a wallet.
  7. Function setParams() allows the owner to apply transaction fees of up to 2% for token transfers to a wallet. The transaction fee limit can be overwritten if the contract implementation is upgraded using the function deprecate().

3.6. Multiple minor observation

Severity: minor observation

Description

  1. Unlocked Pragma.

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.4.17, ensures that contracts do not accidentally get deployed using a compiler version with unfixed bugs.

  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 assessing 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.

  1. 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.

  1. Contract BasicToken inherits the Ownable contract but does not use any of its functionalities and, therefore can safely be removed.
  1. Ownable contract does not use Ownable2Step to transfer ownership of the contract, any mistake in the newOwner address would result in making the functions with the modifier onlyOwner() inaccessible. Consider implementing openzeppelin's Ownable2Step contract to transfer ownerships.
  1. Functions do not emit Events.

Functions transferOwnership(), BRICSChainToken(), issue(), redeem(), and destroyBlackFunds() does not emit event corresponding to the transfer of ownership and token transfers concerning ERC-20 Standard respectively. The ERC-20 standard suggests emitting a Transfer event from/to address(0) when minting or burning tokens for the functions issue(), redeem(), and destroyBlackFunds(). The same applies to the constructor BRICSChainToken().

  1. The BRICSChainToken token's decimals variable does not comply with the ERC-20 Standard, consider using the uint8 data type for the variable.

  2. If the contract is upgraded using the function deprecate() the functions issue(), and redeem() can not be upgraded additionally the functions would be rendered useless. Consider adding upgrade mechanisms to these functions as well.

  1. Consider marking the address parameter in all three blacklist events (DestroyedBlackFunds, AddedBlackList, and RemovedBlackList) as indexed, to allow a client to listen for changes to their status.

  2. Function getOwner() in the contract BlackList is redundant as the function is already inherited from the contract Ownable.

  1. Consider adding require statements as token preconditions to avoid zero transfers for the functions transfer(), transferFrom(), issue(), redeem(), mint(), mintTo(), and burn().
@yuriy77k
Copy link

Token approvals can be changed when the contract state is paused.

It's not an issue, since transferFrom is paused.

@yuriy77k
Copy link

yuriy77k commented Sep 25, 2023 •
edited

3.3. Incorrect short address check.

It's not an issue. Incorrect finding.

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