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

Unrestricted Access Control in setCurves Function of FeeSplitter.sol #64

Closed
c4-bot-6 opened this issue Jan 9, 2024 · 3 comments
Closed
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-4 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-bot-6
Copy link
Contributor

c4-bot-6 commented Jan 9, 2024

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L35

Vulnerability details

Impact

The absence of access control on the setCurves(Curves curves_) function in the FeeSplitter.sol contract presents a significant security risk. This function allows the setting of a new Curves contract instance, which is integral to the operation of the FeeSplitter contract. Since any user can call this function, it opens up the possibility for malicious actors to redirect the contract to a fraudulent Curves contract. This could lead to severe consequences such as misappropriation of funds, incorrect fee calculation and distribution, and overall disruption of the intended functionalities of the FeeSplitter contract within the Curves ecosystem.

Proof of Concept

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L35

The function in question is as follows:

function setCurves(Curves curves_) public {
    curves = curves_;
}

This function lacks any form of access control, making it callable by any external entity.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the setCurves function to include an access control modifier like onlyOwner or onlyManager. This ensures that only authorized addresses can change the Curves contract instance.

Assessed type

Access Control

@c4-bot-6 c4-bot-6 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 9, 2024
c4-bot-10 added a commit that referenced this issue Jan 9, 2024
@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 Jan 16, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #4

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Feb 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Feb 1, 2024

alcueca changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 1, 2024
@thebrittfactor thebrittfactor added 3 (High Risk) Assets can be stolen/lost/compromised directly and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-4 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