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

protocolFeeRecipient can withdraw fee multiple times #266

Closed
code423n4 opened this issue Jan 29, 2023 · 2 comments
Closed

protocolFeeRecipient can withdraw fee multiple times #266

code423n4 opened this issue Jan 29, 2023 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-605 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

Impact

protocolFeeRecipient can withraw fee multiple times without limit until there is no balance in contract

Proof of Concept

The only check applied to withdrawFee function is onlyAdminWithrawAfterEnd wich makes sure the endtime of quest has arrived due that users may claim their rewards after quest endtime the contract should have balance and due that this function can be called multiple times regardless of it's called before or not , even if admin didn't do that others can do at no cost because it's a public function .

Scenario1 : The quest has ended and some users didn't claim their rewards yet , admin calls withdrawFee function and sends protocolFee to feeReceipent and at the same time feeReceipent calls this function and even everyone can do this until there is no balance in contract and users that have not claimed to rewards will not be able to do this . .

Tools Used

Manual Review

Recommended Mitigation Steps

add a state boolean variable for example called feeClaimed and false value in time of construction which indicates fee has been claimed or not then declare a modifier that checks feeClaimed is false otherwise it should revert and add this modifier to withdrawFee function and at the end of the function execution set this variable to true , this way after one time call of this function It's not possible to call it again .

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 29, 2023
code423n4 added a commit that referenced this issue Jan 29, 2023
@c4-judge c4-judge closed this as completed Feb 5, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Feb 5, 2023

kirk-baird marked the issue as duplicate of #23

@c4-judge
Copy link
Contributor

kirk-baird marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 14, 2023
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 duplicate-605 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants