CMTAT - Taurus

The _forcedTransfer() function in the ERC20EnforcementModuleInternal.sol contract is a privileged administrative tool for executing critical transfers, such as moving funds from a frozen account. However, the function lacks a crucial validation check to ensure the transfer amount does not exceed the allowance granted by the from address to the to address. Instead of reverting on insufficient allowance as per the ERC20 standard, the function proceeds with the transfer. This breaks a fundamental security assumption of the ERC20 standard.

While external developer discussions provided acknowledge this behavior, they suggest avoiding the function for such scenarios, which relies on operational policy rather than secure code to prevent misuse. This design allows a mistaken or malicious administrator (DEFAULT_ADMIN_ROLE) to exploit the flawed logic.

The _forcedTransfer() function must be modified to handle allowances in a safe and predictable manner. The logic should be updated to strictly enforce that the transfer amount cannot exceed the existing allowance, causing the transaction to revert if it does.

RISK ACCEPTED: The Taurus team made a business decision to accept the risk of this finding and not alter the contracts, stating:

The goal of the forcedTransfer function is exactly to allow the issuer to transfer tokens without the approval of the token holder. Thus, there is no concept of allowance. The function is distinct from a burn to clearly show the difference between a token supply management operation and an operation that may result from a legal request from the judicial authorities.

It should be noted that in terms of result, this function is no different from the burn function present in the CMTAT as well as the corresponding functions in known tokens such as USDC or USDT.

Since CMTAT is not intended to represent tokens in a defi-friendly way, the administrator is considered trusted. Access to private keys must therefore also be protected accordingly.

The contracts in scope currently use different floating pragma versions ^0.8.0 , ^0.8.22 and ^0.8.28 which means that the code can be compiled by any compiler version that is greater than these versions, and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Additionally, from Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

Lock the pragma version to the same version used during development and testing (for example: pragma solidity 0.8.30;), and make sure to specify the target EVM version when using newly released Solidity versions if deploying to chains that may not support newly introduced opcodes.

Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

ACKNOWLEDGED: The Taurus team made a business decision to acknowledge this finding and not alter the contracts, stating:

One potential use of CMTAT is to be used as a library, similar to OpenZeppelin library.

In this sense, we use the same convention of OpenZeppelin which for the moment only imposes that the version is higher than 0.8.20: pragma solidity ^0.8.20;

A fixed version is set in the config file (0.8.30). Users are free to use these or conduct their own research before switching to another.

The ValidationModuleERC1404 contract is designed to provide human-readable reasons for transfer restrictions, conforming to the ERC-1404 standard. The _detectTransferRestriction() function checks for various conditions like the contract being paused or an address being frozen.

function _detectTransferRestriction(
    address from,
    address to,
    uint256 /* value */
) internal virtual view  returns (uint8 code) {
    if (paused()) {
        return uint8(IERC1404Extend.REJECTED_CODE_BASE.TRANSFER_REJECTED_PAUSED);
    } else if (isFrozen(from)) {
        return uint8(IERC1404Extend.REJECTED_CODE_BASE.TRANSFER_REJECTED_FROM_FROZEN);
    } else if (isFrozen(to)) {
        return uint8(IERC1404Extend.REJECTED_CODE_BASE.TRANSFER_REJECTED_TO_FROZEN);
    } 
    else {
        return uint8(IERC1404Extend.REJECTED_CODE_BASE.TRANSFER_OK);
    }
}

However, the function does not check if the contract has been deactivated. The PauseModule allows for two distinct states: a temporary pause and a permanent deactivation. Since deactivateContract() can only be called when the contract is already paused, the existing paused() check will prevent transfers. The issue is that it will return a TRANSFER_REJECTED_PAUSED code, which is inaccurate and misleading for a contract that has been permanently disabled.

Users and systems interacting with the token would be informed that the contract is temporarily paused, when in reality it has been irreversibly deactivated, creating a discrepancy between the contract's actual state and the reason provided for the transfer failure.

Update the _detectTransferRestriction() function in ValidationModuleERC1404 to check for the deactivated state before checking for the paused state.

This will ensure that the most accurate restriction code is returned.

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

In the __CMTAT_openzeppelin_init_unchained() function of the CMTATBaseCore contract, there is commented out code that is not used. This code may introduce unnecessary confusion to the contract.

// We don'use name and symbol set by the OpenZeppelin module
//__ERC20_init_unchained(ERC20Attributes_.name, ERC20Attributes_.symbol); 

While commenting out code can be useful for debugging or testing purposes, it can also lead to confusion and make the codebase harder to maintain.

Remove the commented-out lines of code to clean up the contract and improve readability.

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

Throughout the codebase, there are several instances of typos in comments. While these typos do not affect the functionality of the code, they can make the codebase harder to read and understand. It is recommended to fix these typos to improve the readability of the codebase.

Instances of this issue include:

• The word contract is misspelled as conract in the ERC20EnforcementModuleInternal contract description.

• The word explanation is misspelled as explaination in the NatSpec for messageForTransferRestriction in ValidationModuleERC1404.

• The word relevant is misspelled as revlevant in a comment in _canMintBurnByModule in ValidationModule.

• The word deactivated is misspelled as deativated in a comment in _canMintBurnByModule in ValidationModule.

• The word spent is misspelled as spended in the NatSpec for transferFrom in ERC20BaseModule.

• The word supplementary is misspelled as supplémentary in a comment in _burnOverride in ERC20BurnModuleInternal.

• The word functions is misspelled as funtions in the library description for EnforcementModuleLibrary.

• The word Solidity is misspelled as Soliditiy in a comment in _batchTransfer in ERC20MintModuleInternal.

It is recommended to fix all typos to improve the readability of the codebase.

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

Several state-changing functions throughout the codebase in scope are currently defined with the public visibility modifier, even though the functions are not called from within the contracts. For functions that are only ever called externally (i.e., not by other functions within the same contract), it is a gas-optimization best practice to use the external visibility modifier.

Instances of this issue include:

• In 0_CMTATBaseCore.sol:

  • function burnAndMint(address from, address to, uint256 amountToBurn, uint256 amountToMint, bytes calldata data) public virtual

  • function forcedBurn(address account, uint256 value, bytes memory data) public virtual

• In 0_CMTATBaseCommon.sol:

  • function burnAndMint(address from, address to, uint256 amountToBurn, uint256 amountToMint, bytes calldata data) public virtual

• In 3_CMTATBaseERC20CrossChain.sol:

  • function crosschainMint(address to, uint256 value) public virtual

  • function crosschainBurn(address from, uint256 value) public virtual

  • function burnFrom(address account, uint256 value) public virtual

  • function burn(uint256 value) public virtual

• In ERC20BaseModule.sol:

  • function setName(string calldata name_) public virtual

  • function setSymbol(string calldata symbol_) public virtual

• In PauseModule.sol:

  • function pause() public virtual

  • function unpause() public virtual

  • function deactivateContract() public virtual

• In EnforcementModule.sol:

  • function setAddressFrozen(address account, bool freeze) public virtual

  • function setAddressFrozen(address account, bool freeze, bytes calldata data) public virtual

  • function batchSetAddressFrozen(address[] calldata accounts, bool[] calldata freezes) public virtual

• In ERC20MintModule.sol:

  • function mint(address account, uint256 value) public virtual

  • function batchMint(address[] calldata accounts, uint256[] calldata values) public virtual

  • function batchTransfer(address[] calldata tos, uint256[] calldata values) public

• In ERC20BurnModule.sol:

  • function burn(address account, uint256 value) public virtual

  • function batchBurn(address[] calldata accounts, uint256[] calldata values, bytes memory data) public virtual

  • function batchBurn(address[] calldata accounts, uint256[] calldata values) public virtual

• In ERC20EnforcementModule.sol:

  • function forcedTransfer(address from, address to, uint256 value, bytes calldata data) public virtual

  • function forcedTransfer(address from, address to, uint256 value) public virtual

  • function freezePartialTokens(address account, uint256 value) public virtual

  • function unfreezePartialTokens(address account, uint256 value) public virtual

  • function freezePartialTokens(address account, uint256 value, bytes calldata data) public virtual

  • function unfreezePartialTokens(address account, uint256 value, bytes calldata data) public virtual

• In ExtraInformationModule.sol:

  • function setTokenId(string calldata tokenId_) public virtual

  • function setTerms(IERC1643CMTAT.DocumentInfo calldata terms_) public virtual

  • function setInformation(string calldata information_) public virtual

• In SnapshotEngineModule.sol:

  • function setSnapshotEngine(ISnapshotEngine snapshotEngine_) public virtual

• In ValidationModuleRuleEngine.sol:

  • function setRuleEngine(IRuleEngine ruleEngine_) public virtual

• In AllowlistModule.sol:

  • function setAddressAllowlist(address account, bool status) public virtual

  • function setAddressAllowlist(address account, bool status, bytes calldata data) public virtual

  • function batchSetAddressAllowlist(address[] calldata accounts, bool[] calldata status) public virtual

  • function enableAllowlist(bool status) public virtual

• In DebtEngineModule.sol:

  • function setDebtEngine(IDebtEngine debtEngine_) public virtual

• In DebtModule.sol:

  • function setCreditEvents(CreditEvents calldata creditEvents_) public

  • function setDebt(ICMTATDebt.DebtInformation calldata debt_) public virtual

  • function setDebtInstrument(ICMTATDebt.DebtInstrument calldata debtInstrument_) public virtual

• In ERC7551Module.sol:

  • function setMetaData(string calldata metadata_) public virtual

  • function setTerms(bytes32 hash, string calldata uri) public virtual

Modify the public functions not used within the contracts with the external visibility modifier.

ACKNOWLEDGED: The Taurus team made a business decision to acknowledge this finding and not alter the contracts, stating:

According to RareSkills optimization book, section Outdated tricks, suing the keyword external instead of public is no longer an optimization in terms of gas.

Via the public keyword, this allows users of the library to override the function in their contra to change its behavior when needed.

In the _unfreezeTokens function within the ERC20EnforcementModuleInternal.sol contract, a comment incorrectly describes the relationship between frozen tokens and the total balance. The comment states: // Frozen token can not be < balance.

This comment is misleading because the number of frozen tokens for an account can, and typically will, be less than the account's total balance. The actual invariant that is maintained is that the frozen token amount cannot be greater than the total balance, which is enforced in the _freezePartialTokens function.

While this does not introduce a direct vulnerability, it can cause confusion for developers and auditors, potentially leading to incorrect assumptions about the contract's logic.

It is recommended to correct the comment to accurately reflect the code's logic. This will improve the clarity and maintainability of the code.

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

The ERC20EnforcementModuleInternal contract calls the balanceOf function from its parent ERC20Upgradeable contract using two different syntaxes. The _freezePartialTokens function uses a direct call, balanceOf(account), while the _getActiveBalanceOf function uses an explicit call to the parent contract, ERC20Upgradeable.balanceOf(account).

While both calls are functionally equivalent in the current implementation, this inconsistency can reduce code clarity and introduce potential maintenance challenges. Future developers might be confused about whether there is a deliberate reason for the different call styles, which could lead to errors if the contract is extended or modified.

For improved code consistency and readability, adopt a single, uniform method for calling functions from parent contracts throughout the codebase.

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

The project contains several unnamed mappings despite using a Solidity version that supports named mappings.

Named mappings improve code readability and self-documentation by explicitly stating their purpose.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit. For example:

mapping(address myAddress => bool myBool) public myMapping;

SOLVED: The Taurus team solved this finding in the specified commit by following the mentioned recommendation.

The file 0_CMTATBaseGeneric.sol exists within the project's codebase. However, it is not imported, inherited, or otherwise utilized by any other contract in the system.

The presence of such "dead code" can increase the complexity of the project, leading to potential confusion for future developers and auditors who may spend time analyzing code that has no impact on the protocol's logic. Maintaining a clean and concise codebase is essential for security and long-term maintainability.

Remove the 0_CMTATBaseGeneric.sol file from the project to improve code hygiene.

ACKNOWLEDGED: The Taurus team made a business decision to acknowledge this finding and not alter the contracts, stating:

The file 0_CMTATBaseGeneric.sol exists to allow CMTAT users to use CMTAT code with nonstandard ERC-20 token, for example ERC-721 token or Zama FHE ERC-20 encrypted tokens.

While CMTAT does not provide a deployment version using this, functionnalities are tested through an ERC-721 mock contract ERC721Upgradeable.