DOMAIN_SEPARATOR Incorrect signature content

[c4-submissions]

Lines of code

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

Vulnerability details

Vulnerability details

In the constructor method of ERC20.SOL, _DOMAIN_SEPARATOR will be generated by _calculateDomainSeparator() and this variable will be used as part of the signature in permit().

The main method is as follows.

contract ERC20 is Context {
...
    constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

        deploymentChainId = block.chainid;
@>      _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }

    function _calculateDomainSeparator(uint256 chainId) private view returns (bytes32) {
        return keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
@>?             keccak256(bytes(name)),
                keccak256(bytes(version)),
                chainId,
                address(this)
            )
        );
    }

    function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
        require(block.timestamp <= deadline, "ERC20/permit-expired");
        require(owner != address(0), "ERC20/invalid-owner");

        uint256 nonce;
        unchecked {
            nonce = nonces[owner]++;
        }
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
@>              block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
            )
        );

        require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

        allowance[owner][spender] = value;
        emit Approval(owner, spender, value);
    }

the content of _DOMAIN_SEPARATOR contains the token's name

The problem is that there are two issues with this token.name.

1. token.name is not initialized in the constructor, it needs to be assigned by file()

2. poolManager.updateTrancheTokenMetadata() can modify token.name later.

so _DOMAIN_SEPARATOR will contain a blank name , it's wrong.

Because name can change, the immutable nature of _DOMAIN_SEPARATOR is infeasible and it cannot be used as part of a signature.

It is recommended not to use _DOMAIN_SEPARATOR, but to calculate it in real-time with _calculateDomainSeparator().

Impact

The user assembles the content of the signature through the EIP-712 standard, but cannot execute permit() properly.

Proof of Concept

The following code demonstrates this, assembling the signature content as standard, but not the same as token.DOMAIN_SEPARATOR()

add to Tranche.t.sol

    function test_NoEqualDomainSeparator() public {
        bytes32 tokenDomainSeparator = token.DOMAIN_SEPARATOR();
        bytes32 calculateDomainSeparator = keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                keccak256(bytes(token.name())),
                keccak256(bytes(token.version())),
                block.chainid,
                address(token)
            )
        );
      
        console.log("equal:",tokenDomainSeparator == calculateDomainSeparator);
    }

 $ forge test -vvv --match-test test_NoEqualDomainSeparator

Running 1 test for test/token/Tranche.t.sol:TrancheTokenTest
[PASS] test_NoEqualDomainSeparator() (gas: 14966)
Logs:
  equal: false

Recommended Mitigation

remove _DOMAIN_SEPARATOR , calculate it in real-time with _calculateDomainSeparator().

contract ERC20 is Context {
...

-   uint256 public immutable deploymentChainId;
-   bytes32 private immutable _DOMAIN_SEPARATOR;

    constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

-       deploymentChainId = block.chainid;
-       _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }


    function DOMAIN_SEPARATOR() external view returns (bytes32) {
-       return block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid);
+       return _calculateDomainSeparator(block.chainid);
    }

    function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
        require(block.timestamp <= deadline, "ERC20/permit-expired");
        require(owner != address(0), "ERC20/invalid-owner");

        uint256 nonce;
        unchecked {
            nonce = nonces[owner]++;
        }
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
-               block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
+               _calculateDomainSeparator(block.chainid), 
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
            )
        );

        require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

        allowance[owner][spender] = value;
        emit Approval(owner, spender, value);
    }

Assessed type

Context

[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