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

ERC20.permit() may not work because wrong calculation #345

Closed
c4-submissions opened this issue Sep 14, 2023 · 3 comments
Closed

ERC20.permit() may not work because wrong calculation #345

c4-submissions opened this issue Sep 14, 2023 · 3 comments
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 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

Due to wrong calculation of _DOMAIN_SEPARATOR, ERC20.permit() may not verify signature signed by signer. The design is supposed to support offline token approving but can not work as expected.

Proof of Concept

When calculating _DOMAIN_SEPARATOR during tranche token deployment, the field name is also included, but the value is not set in constructor(). It only has correct value when ERC20.file("name", name) is called.

From the user's perspective, they will create signature following EIP-2612, the name of the tranche token will be used for _DOMAIN_SEPARATOR calculation and the result will be different from the value stored in tranche token smart contract, ERC20.permit() will always be reverted because of this incorrect calculation.

Tools Used

Manual review

Recommended Mitigation Steps

Pass name in ERC20.constructor() other than assigning it's value through ERC20.file("name", name):

constructor(string memory _name, string memory _symbol, uint8 decimals_) {
    name = _name,
    symbol = _symbol,
    decimals = decimals_;
    wards[_msgSender()] = 1;
    emit Rely(_msgSender());

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

Assessed type

Other

@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 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

3 participants