Tranche is not compliant with EIP-1404

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/Tranche.sol#L35-L39
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/Tranche.sol#L72-L73

Vulnerability details

Impact

Tranche Token is defined as: "Extension of ERC20 + ERC1404 for tranche tokens"
It utilizes RestrictionManager.sol - which, according to provided documentation is: ERC1404 based contract that checks transfer restrictions.

However, there are some deviations from EIP-1404 in its implementation, which implies, that Tranche Token is not compliant with EIP-1404.

This may cause unexpected behavior due to being non compliant with EIP-1404. Other protocols that integrate with contract may incorrectly assume that it's EIP-1404 compliant - especially that documentation states that it's ERC-1404. EIP-1404 purpose is to create a robust and consistent implementation patterns. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-1404, it should fully conform to EIP-1404 standard.

During the previous Code4rena, lack of EIP compliance were evaluated as High/Medium:

• (Lack of EIP-4626 compliance) Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47 - has been evaluated as High
• (Lack of EIP-4524 compliance) HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524 2022-10-holograph-findings#440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) HolographERC721.safeTransferFrom not compliant with EIP-721 2022-10-holograph-findings#203 - has been evaluated as Medium

Taking into consideration the current risk of this issue, we've evaluated it as Medium.
The severity evaluation has been prepared with help of Code4rena Severity Categorization:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Since the "hypothetical attack path with stated assumptions" occurs (external protocol which integrates with contract may assume it's EIP-1404 compliant) - the severity has been evaluated as Medium only.

Proof of Concept

According to EIP-1404:

The only requirement is that detectTransferRestriction must be evaluated inside a token's transfer and transferFrom methods.

EIP-1404 requires, that detectTransferRestriction is being evaluated inside transfer and transferFrom.
However, current implementation of Tranche Tokens evaluates detectTransferRestriction also inside mint:

    modifier restricted(address from, address to, uint256 value) {
        uint8 restrictionCode = detectTransferRestriction(from, to, value);
        require(restrictionCode == restrictionManager.SUCCESS_CODE(), messageForTransferRestriction(restrictionCode));
        _;
    }

    function mint(address to, uint256 value) public override restricted(_msgSender(), to, value) {
        return super.mint(to, value);
    }

This may cause unexpected behavior and potential fund loss. Other protocols that integrate with contract may incorrectly assume that detectTransferRestriction is being called only for transfer and transferFrom - as EIP-1404 requires. However, this function is also evaluted during mint(). Other protocols that integrate with contract may try to call mint(), without knowing that detectTransferRestriction may revert during mint. Due to lack of EIP-1404 compliance, funds may be lost - since minting won't be possible:

1. External contract integrates with protocol, assuming it's compliant with EIP-1404
2. Contract calls mint, without realizing, that mint evalutes detectTransferRestriction (even though, EIP-1404 requires that detectTransferRestriction evaluates only for transferFrom and transfer).
3. Because of detectTransferRestriction - mint() fails.
4. Funds loss occur - since new tokens haven't been minted.

Tools Used

Manual code review

Recommended Mitigation Steps

Do not evaluate detectTransferRestriction on mint() call, as - according to EIP-1404, this function should be evaluated on transfer() and transferFrom() calls. If mint() has the same restrictions as transfer and transferFrom - implement additional modifier - or code them directly inside mint().

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #233

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 removed the grade

[hieronx]

This is intended, and does not make the token non-compliant. ERC1404 just requires that transfer/transferFrom include this check, it does not require that ONLY these methods implement it.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid