GoGoPool contest - ak1's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L196-L269

Vulnerability details

Impact

owner will be deprived from picking the node id which they already used.

when natspec says // If nodeID exists, only allow overwriting if node is finished or canceled

The pool that are marked as Withdrawable and Error are also used to overwrite the node id

finished will be set in withdrawMinipoolFunds where the staked amount is reduced but avaxassigned is not reset. This would be drawback to owner of nodeID

function withdrawMinipoolFunds(address nodeID) external nonReentrant {
	int256 minipoolIndex = requireValidMinipool(nodeID);
	address owner = onlyOwner(minipoolIndex);
	requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
	setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));


	uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")));
	uint256 avaxNodeOpRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")));
	uint256 totalAvaxAmt = avaxNodeOpAmt + avaxNodeOpRewardAmt;


	Staking staking = Staking(getContractAddress("Staking"));
	staking.decreaseAVAXStake(owner, avaxNodeOpAmt);


	Vault vault = Vault(getContractAddress("Vault"));
	vault.withdrawAVAX(totalAvaxAmt);
	owner.safeTransferETH(totalAvaxAmt);
}

Proof of Concept

createMinipool function is public and anyone can call it.

Here, the overwrite can happen which does not check for owner of the nodeID.

	// If nodeID exists, only allow overwriting if node is finished or canceled
	// 		(completed its validation period and all rewards paid and processing is complete)
	int256 minipoolIndex = getIndexOf(nodeID);
	if (minipoolIndex != -1) {
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
		resetMinipoolData(minipoolIndex);
		// Also reset initialStartTime as we are starting a whole new validation
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);

Also, when we look at this line requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch), Prelaunch state can be allowed for Withdrawable,Error pools also. The comment from dev says that only finished or canceled pools would be allowed for overwrte but that is not the case here.

function requireValidStateTransition

	} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
		isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
	} else if (currentStatus == MinipoolStatus.Finished || currentStatus == MinipoolStatus.Canceled) {
		// Once a node is finished/canceled, if they re-validate they go back to beginning state
		isValid = (to == MinipoolStatus.Prelaunch);
	} else {

Tools Used

Manual review

Recommended Mitigation Steps

check for owner of the nodeid using the function onlyOwner before overwrite

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L166-L178

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares.

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Proof of Concept

The depositAVAX is,

function depositAVAX() public payable returns (uint256 shares) {
	uint256 assets = msg.value;
	// Check for rounding error since we round down in previewDeposit.
	if ((shares = previewDeposit(assets)) == 0) {
		revert ZeroShares();
	}


	emit Deposit(msg.sender, msg.sender, assets, shares);


	IWAVAX(address(asset)).deposit{value: assets}();
	_mint(msg.sender, shares);
	afterDeposit(assets, shares);
}.

previewDeposit calles the convertToShares

function previewDeposit(uint256 assets) public view virtual returns (uint256) {
    return convertToShares(assets);
}

The convertToShares function is

function convertToShares(uint256 assets) public view virtual returns (uint256) {
    uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.


    return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}

Tools Used

Manual review

Recommended Mitigation Steps

onsider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L155-L197

Vulnerability details

Impact

when the contract is paused , allowing startRewardsCycle would inflate the token value which might not be safe.

Rewards should not be claimed by anyone when all other operations are paused.

I saw that the witdrawGGP has this WhenNotPaused modifier.

Inflate should not consider the paused duration.

lets say, when the contract is paused for theduration of 2 months, then the dao, protocol, and node validator would enjoy the rewards. This is not good for a health protocol

Proof of Concept

startRewardsCycle does not have the WhenNotPaused modifier.

function startRewardsCycle() external {
	if (!canStartRewardsCycle()) {
		revert UnableToStartRewardsCycle();
	}


	emit NewRewardsCycleStarted(getRewardsCycleTotalAmt());


	// Set start of new rewards cycle
	setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp);
	increaseRewardsCycleCount();
	// Mint any new tokens from GGP inflation
	// This will always 'mint' (release) new tokens if the rewards cycle length requirement is met
	// 		since inflation is on a 1 day interval and it needs at least one cycle since last calculation
	inflate();


	uint256 multisigClaimContractAllotment = getClaimingContractDistribution("ClaimMultisig");
	uint256 nopClaimContractAllotment = getClaimingContractDistribution("ClaimNodeOp");
	uint256 daoClaimContractAllotment = getClaimingContractDistribution("ClaimProtocolDAO");
	if (daoClaimContractAllotment + nopClaimContractAllotment + multisigClaimContractAllotment > getRewardsCycleTotalAmt()) {
		revert IncorrectRewardsDistribution();
	}


	TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
	Vault vault = Vault(getContractAddress("Vault"));


	if (daoClaimContractAllotment > 0) {
		emit ProtocolDAORewardsTransfered(daoClaimContractAllotment);
		vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment);
	}


	if (multisigClaimContractAllotment > 0) {
		emit MultisigRewardsTransfered(multisigClaimContractAllotment);
		distributeMultisigAllotment(multisigClaimContractAllotment, vault, ggp);
	}


	if (nopClaimContractAllotment > 0) {
		emit ClaimNodeOpRewardsTransfered(nopClaimContractAllotment);
		ClaimNodeOp nopClaim = ClaimNodeOp(getContractAddress("ClaimNodeOp"));
		nopClaim.setRewardsCycleTotal(nopClaimContractAllotment);
		vault.transferToken("ClaimNodeOp", ggp, nopClaimContractAllotment);
	}
}

Tools Used

Manual review

Recommended Mitigation Steps

We suggest to use WhenNotPaused modifier.

Lines of code

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

Vulnerability details

Impact

There is whenNotPaused in function stakeGGP(uint256 amount) external whenNotPaused {.

This can be added by team to prevent the staking for a particular duration.

The scenario could be where the paused is desired. but this can be bypassed. 1. For maintenance. 2. there could be some issue due to staking. whether protocol or user could suffer if they stake so the whenNotPaused can be used to prevent this.

already staked user can use the restakeGGP route to stake even when the stakeGGP is paused.

Proof of Concept

Below is the stakeGGP where the whenNotPaused is used.

function stakeGGP(uint256 amount) external whenNotPaused {
	// Transfer GGP tokens from staker to this contract
	ggp.safeTransferFrom(msg.sender, address(this), amount);
	_stakeGGP(msg.sender, amount);
}.

Below is the restakeGGP which does not have the whenNotPaused

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);
} 

Tools Used

Manual review

Recommended Mitigation Steps

Use the whenNotPaused for restakeGGP too.