ERC20 Token Implementation: Name change could result in signature verification failure because of the domain separator mismatch

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L84
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L48

Vulnerability details

Summary

This report addresses an issue in the ERC20 token implementation, specifically concerning the ability to change the token's name and symbol. While the implementation adheres to the EIP 712 standard for signing permit functions, it has a crucial flaw related to the handling of the token's name. The issue arises when the token's name is changed after the initial setup, leading to incorrect calculations within the DOMAIN_SEPARATOR of the contract. Consequently, this impacts the ability to recover signatures correctly, causing potential failures during signature verification.
Something more - current implementation uses empty string for name in the DOMAIN_SEPARATOR, because of the factory implementation.

Impact

The impact of this issue is not considered high in terms of security, as it does not compromise the integrity of the token's funds or directly expose vulnerabilities to malicious actors. However, it does introduce undesirable consequences. Changing the name of a token should ideally result in the recalibration of the DOMAIN_SEPARATOR to maintain consistency. Failing to do so creates confusion and breaks the standard defined by EIP 712.

The EIP 712 standard specifies that the 'domainName' field within domainSeparator should represent the user-readable name of the signing domain, which includes the DApp or protocol name. When the token's name is changed without updating the DOMAIN_SEPARATOR, it deviates from this standard.

Proof of Concept

NOTE: 1 and 2 from below are different scenarios and for current implementation 1st will always override 2nd

1. Current factory implementation first deploy a tranche token (ERC20) and after that sets its name and symbol. But the DOMAIN_SEPARATOR of the token is calculated on deployment and no name is passed in the constructor, it will result in using empty string.
   Here are the steps and why in the DOMAIN_SEPARATOR of each token 'name' would be empty string, which is undesired behaviour.

• PoolManager deploy new tranche token
• TrancheTokenFactory creates new TrancheToken instance only passing decimals
• TrancheToken is initialised and DOMAIN_SEPARATOR is calculated using empty name
• On next lines factory sets name and symbol, but immutable DOMAIN_SEPARATOR is already with empty name

2. Initially, the name of the token is included inside the DOMAIN_SEPARATOR of the contract. This is calculated only once, during the contract's deployment

• If the token's name is subsequently changed using file function

• In this scenario, if the original name of the token was "Test token" and it was later changed to "Better name token," a signer would utilise "Better name token" for the domainName value in the hash struct used for the domainSeparator. This change could lead to a perpetual failure in the ecrecover signature verification process, if the signer follows EIP712 standard process, even if all other data remains valid.

Tools Used

Manual Review

Recommended Mitigation Steps

• Consider initialising a token using name as required field and removing the option to modify the name and symbol of a token after the initial setup, or
• Implement a mechanism to recalculate the DOMAIN_SEPARATOR when the name is changed to maintain consistency with the EIP 712 standard.
  While recalculating the DOMAIN_SEPARATOR is a possible solution, it may introduce complexity and potential issues. Therefore, careful consideration should be given to the best approach for your specific use case and design requirements.

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #146

[c4-judge]

gzeon-c4 marked the issue as satisfactory