Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_DOMAIN_SEPARATOR is not updated, when name of token is changed #404

Closed
c4-submissions opened this issue Sep 14, 2023 · 3 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-146 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Sep 14, 2023

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L84
https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L223

Vulnerability details

Impact

_DOMAIN_SEPARATOR is not updated, when name of token is changed

Proof of Concept

This is similar to the one that i have already submited. However, i believe that this is separate issue, because in case if first one is fixed, this one still remains. While first bug is about wrong initialization, this one is about updating metadata of contract which is a part of functionality of protocol.
Here is similar issue on c4.

According to the EIP-712 in case if name, version or any other fields in the _DOMAIN_SEPARATOR is changed, then all previous signatures should not be valid anymore.

Tranche token metadata can be changed if needed. This is handled by PoolManager.updateTrancheTokenMetadata function. This function will change name field, which is part of _DOMAIN_SEPARATOR.
The problem is that after changing the name field, _DOMAIN_SEPARATOR will not be recalculated, which means that all signatures that were signed before will be still valid. Also as i have described in another report, user's will see that Tranche token name is not same as in the domain separator that they should sign, so users will likely reject signing. And people who will craft messages themselves will use real updated name to create domain separator, so it will not match and permit will revert.

Tools Used

VsCode

Recommended Mitigation Steps

When you update name, then recalculate domain separator.

Assessed type

Error

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 14, 2023
c4-submissions added a commit that referenced this issue Sep 14, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 15, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #146

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 26, 2023
@c4-judge
Copy link

gzeon-c4 marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-146 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants