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

Owner can lock tokens in MasterChef #238

Open
code423n4 opened this issue Feb 9, 2022 · 1 comment
Open

Owner can lock tokens in MasterChef #238

code423n4 opened this issue Feb 9, 2022 · 1 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

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L82-L84

Vulnerability details

Impact

Owner can remove a depositor. Since only depositors can deposit and withdraw, the owner may add a contract to the whitelist, let users deposit in the contarct and remove the depositor from the whitelist. Depositor's reward cannot be withdrawn then. And takes a share of Concur tokens that will not be ditributed.

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove onlyDepositor modifier from the withdraw function.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 9, 2022
code423n4 added a commit that referenced this issue Feb 9, 2022
@r2moon r2moon added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 16, 2022
@leekt leekt added invalid This doesn't seem right and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Feb 18, 2022
@GalloDaSballo
Copy link
Collaborator

The finding is valid in that the sponsor / owner can remove all depositors.

I believe having an immutable depositor that can't be changed would give stronger security guarantees.

While the finding is valid, because it is contingent on a malicious owner, I believe Medium Severity to be more appropriate

@GalloDaSballo GalloDaSballo added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Apr 12, 2022
@GalloDaSballo GalloDaSballo removed the invalid This doesn't seem right label Apr 24, 2022
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
Projects
None yet
Development

No branches or pull requests

4 participants