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

Staking.restakeGGP should not be allowed when contract is paused #236

Closed
code423n4 opened this issue Dec 29, 2022 · 4 comments
Closed

Staking.restakeGGP should not be allowed when contract is paused #236

code423n4 opened this issue Dec 29, 2022 · 4 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 downgraded by judge Judge downgraded the risk level of this issue duplicate-673 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L328-L332
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L106

Vulnerability details

Impact

Staking.restakeGGP should not be allowed when contract is paused. Even that it's only callable by ClaimNodeOp, it's still can be directly called by user through ClaimNodeOp.claimAndRestake.

Proof of Concept

When Staking contract is paused, then staking and withdrawing of GGP is disallowed. So both stakeGGP and withdrawGGP functions have whenNotPaused modifier.
Also there is another possibility to stake GGP using Staking.restakeGGP function. It's only callable by ClaimNodeOp. And this function doesn't have whenNotPaused modifier.
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L328-L332

	function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
		// Transfer GGP tokens from the ClaimNodeOp contract to this contract
		ggp.safeTransferFrom(msg.sender, address(this), amount);
		_stakeGGP(stakerAddr, amount);
	}

restakeGGP can be called only by ClaimNodeOp. It is called inside ClaimNodeOp.claimAndRestake function.
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L89-L114

	function claimAndRestake(uint256 claimAmt) external {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 ggpRewards = staking.getGGPRewards(msg.sender);
		if (ggpRewards == 0) {
			revert NoRewardsToClaim();
		}
		if (claimAmt > ggpRewards) {
			revert InvalidAmount();
		}


		staking.decreaseGGPRewards(msg.sender, ggpRewards);


		Vault vault = Vault(getContractAddress("Vault"));
		uint256 restakeAmt = ggpRewards - claimAmt;
		if (restakeAmt > 0) {
			vault.withdrawToken(address(this), ggp, restakeAmt);
			ggp.approve(address(staking), restakeAmt);
			staking.restakeGGP(msg.sender, restakeAmt);
		}


		if (claimAmt > 0) {
			vault.withdrawToken(msg.sender, ggp, claimAmt);
		}


		emit GGPRewardsClaimed(msg.sender, claimAmt);
	}

As you can see this function can be called by anyone who has ggp rewards.
So using ClaimNodeOp.claimAndRestake user can dismiss restriction for staking ggp when Staking contract is paused.

Tools Used

VsCode

Recommended Mitigation Steps

Add whenNotPaused modifier to Staking.restakeGGP function.

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

c4-judge commented Jan 8, 2023

GalloDaSballo marked the issue as duplicate of #351

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #673

@c4-judge
Copy link
Contributor

c4-judge commented Feb 8, 2023

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 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 8, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Feb 8, 2023

GalloDaSballo changed the severity to 2 (Med Risk)

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 downgraded by judge Judge downgraded the risk level of this issue duplicate-673 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants