GoGoPool contest - AkshaySrivastav's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L379-L383

Vulnerability details

Impact

The protocol docs mentions that "If the validator is failing at their duties, their GGP will be slashed and used to compensate the loss to our Liquid Stakers."

But the actual implementation of the Staking.slashGGP function is very different from the above specification.

	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		Vault vault = Vault(getContractAddress("Vault"));
		decreaseGGPStake(stakerAddr, ggpAmt);
		vault.transferToken("ProtocolDAO", ggp, ggpAmt);
	}

The few issues in the above code can be listed as:

• The contracts do not distribute the slashed amount to Liquid stakers or ggAVAX holders (unlike as stated in the docs).
• The contract simply transfer the GGP token amount to ProtocolDAO contract via Vault. Currently ProtocolDAO contract do not contain any function to utilize or recover those transferred GGP tokens. To utilize the GGP funds the ProtocolDAO contract will be needed to be upgraded.

Proof of Concept

Consider this scenario:

1. A node operator do not maintains sufficient uptime and hence the multisig decides to slash his GGP stake.
2. The multisig invokes MinipoolManager.recordStakingEnd function which invokes the Staking.slashGGP function.
3. The slashed rewards get sent to ProtocolDAO contract via Vault. This GGP balance of ProtocolDAO contract in Vault simply sits idle and unusable until the ProtocolDAO contract is upgraded with logic to utilize that GGP balance.

Tools Used

Manual review

Recommended Mitigation Steps

The sponsors should re-implelment the Staking.slashGGP function with alignment to the protocol docs so that the contract natively distribute the slashed GGP to liquid stakers. Or, The ProtocolDAO contract should be modified so that it can natively handle the transferred GGP token balance.

Lines of code

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

Vulnerability details

Impact

The MinipoolManager.createMinipool function do not validate the caller's address due to which any address can invoke the createMinipool function with any nodeID (existing or new) as input. For any existing nodeID the function can be invoked as long as the requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch) condition is met. This condition is met when the Minipool is in these states:

• Withdrawable
• Finished
• Canceled
• Error

Upon execution the createMinipool function resets the Minipool state (sets all values to zero).

Since the function can be invoked by anyone for any existing nodeID, it can be used to nullify all funds of node operators including original AVAX staked plus any AVAX rewards. The attacker will need some initial capital to invoke the createMinipool function, this capital however can be safely recovered by the attacker after MinipoolCancelMoratoriumSeconds duration.

This attack can result in loss of funds for all the node operators of GoGoPool protocol.

Proof of Concept

Steps for exploit:

1. The attacker waits until a Minipool (of nodeID N) comes into Withdrawable state.
2. Attacker stakes the necessary GGP and invokes the createMinipool function with nodeID N as input.
3. The Minipool state gets reset hence all funds of original Minipool owner (node operator) gets resets to 0. Also the attacker becomes the owner of that respective Minipool.
4. After waiting for MinipoolCancelMoratoriumSeconds the attacker can successfully recover his initial capital.

Test Scenario:

contract MinipoolManagerTest is BaseTest {
	using FixedPointMathLib for uint256;
	address private nodeOp;
	address private attacker;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
		attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);
	}

	function testBug() public {
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;

		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

		skip(duration);

		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
		vm.stopPrank();

		uint256 attackerInitialAvaxBal = attacker.balance;
		uint256 attackerInitialGgpBal = ggp.balanceOf(attacker);
		vm.startPrank(attacker);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(200 ether);
		minipoolMgr.createMinipool{value: 1000 ether}(mp1.nodeID, 0, 0, 1000 ether);
		vm.stopPrank();

		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
		minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
		assertEq(minipoolMgr.getMinipool(minipoolMgr.getIndexOf(mp1.nodeID)).owner, attacker);
		vm.stopPrank();

		skip(dao.getMinipoolCancelMoratoriumSeconds());
		vm.startPrank(attacker);
		minipoolMgr.cancelMinipool(mp1.nodeID);
		staking.withdrawGGP(200 ether);

		assertEq(attacker.balance, attackerInitialAvaxBal);
		assertEq(ggp.balanceOf(attacker), attackerInitialGgpBal);
	}
}

Tools Used

Manual review

Recommended Mitigation Steps

The createMinipool function should validate the caller using the onlyOwner function.

Also the sponsors should verify whether it is intended to allow the Minipool state transition from Withdrawable to Prelaunch state. If not, the else if condition in requireValidStateTransition should be corrected accordingly.

             else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
		}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L209-L216

Vulnerability details

Impact

The ProtocolDAO.upgradeExistingContract is intended to register a new contract in protocol and unregister the old contract. It essentially combined registerContract and unregisterContract function calls in a single call.

	function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

According to its implementation it can be seen that it first registers the new contract and then unregisters the old one. This sequence causes issues if the new and old name of the contract is same. In that case the storage values gets messed up.

Proof of Concept

contract BugTest is Test {
	Storage public store;
	ProtocolDAO public dao;

	function setUp() public {
		store = new Storage();
		dao = new ProtocolDAO(store);
		store.setBool(keccak256(abi.encodePacked("contract.exists", address(dao))), true);
        }

        function test_upgradeExistingContract() public {
        	string memory contractName = "Oracle";
        	address existingAddr = address(0x100);

        	dao.registerContract(existingAddr, contractName);

        	address newAddr = address(0x200);
        	dao.upgradeExistingContract(newAddr, contractName, existingAddr);

        	assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", existingAddr))), false);
		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", contractName))), address(0));
		assertEq(store.getString(keccak256(abi.encodePacked("contract.name", existingAddr))), "");
        	assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", newAddr))), true);
		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", contractName))), address(0));
		assertEq(store.getString(keccak256(abi.encodePacked("contract.name", newAddr))), "Oracle");
    	}
}

In the test case above, the newAddr address value points to "Oracle" string but the "Oracle" string points to null address.

Tools Used

Manual review

Recommended Mitigation Steps

Consider performing unregistering the old contract before registering the new one or consider validating that new and old contract names cannot be same.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Ocyticus.sol#L55 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L57

Vulnerability details

Impact

In GoGoPool protocol, all multisigs can be disabled using the Ocyticus.pauseEverything and Ocyticus.disableAllMultisigs functions. It should also be noted that in MinipoolManager every Minipool gets assigned a mulstisig at the time of that Minipool creation.

After a multisig has been disabled it can still perform various operations on the Minipool for which it was assigned as a valid multisig. The operations includes:

• claimAndInitiateStaking
• recordStakingStart
• recordStakingEnd
• recreateMinipool (when not paused)
• recordStakingError
• cancelMinipoolByMultisig
• finishFailedMinipoolByMultisig

The disabling of multisigs can be done for various reasons (including private key compromises) and letting disabled multisigs perform crucial operations on Minipools is not ideal.

Proof of Concept

Consider this scenario:

• A multisig M1 was assigned to a Minipool.
• Then Ocyticus.disableAllMultisigs was invoked and all multisigs were disabled.
• Still the multisig M1 can invoke various functions for the Minipool (claimAndInitiateStaking, recordStakingEnd, recreateMinipool, recordStakingError, cancelMinipoolByMultisig, finishFailedMinipoolByMultisig) which can cause unintended loss of funds to the users or can result in ambiguous state for the protocol contracts.

Tools Used

Manual review

Recommended Mitigation Steps

The protocol should check the current enabled or disabled state of the caller multisigs before allowing it to perform any operation on the Minipool. The protocol should also have a way to upgrade the assigned multisig for a Minipool.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L664

Vulnerability details

Impact

The MinipoolManager._cancelMinipoolAndReturnFunds function implements a push payment mechanism for ETH (AVAX) transfers. This function is internally called by cancelMinipoolByMultisig function.

A malicious contract which reverts on all plain ETH transfer can be used to prevent a multisig from canceling the Minipool. Since the Minipool now cannot move to Canceled state, the only state possible forward for the Minipool is the Launched state or just be in the Prelaunch state forever. Both the scenarios are completed unintentional and unexpected for the MinipoolManager contract.

Proof of Concept

Test case:

contract Malicious {
	receive() external payable { revert("Not Receivable"); }
	function stakeGGP(address staking, ERC20 ggp, uint256 amount) public {
		ggp.approve(staking, amount);
		Staking(staking).stakeGGP(amount);
	}
	function createMinipool(address manager, address nodeID, uint256 avaxAssignmentRequest) public payable {
		MinipoolManager(manager).createMinipool{value: msg.value}(nodeID, 0, 0, avaxAssignmentRequest);
	}
}

contract MinipoolManagerTest is BaseTest {
	address private attacker;
	function setUp() public override {
		super.setUp();
		attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);
	}

	function testBug() public {
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint128 ggpStakeAmt = 200 ether;
		address nodeID = address(1009);

		Malicious maliciousC = new Malicious();

		vm.startPrank(attacker);
		ggp.transfer(address(maliciousC), MAX_AMT);
		maliciousC.stakeGGP(address(staking), ggp, ggpStakeAmt);
		maliciousC.createMinipool{value: depositAmt}(address(minipoolMgr), nodeID, avaxAssignmentRequest);
		vm.stopPrank();

		bytes32 errorCode = "INVALID_NODEID";
		vm.prank(address(rialto));
		vm.expectRevert("ETH_TRANSFER_FAILED");
		minipoolMgr.cancelMinipoolByMultisig(nodeID, errorCode);
	}
}

Tools Used

Manual review

Recommended Mitigation Steps

The MinipoolManager._cancelMinipoolAndReturnFunds should implement a pull payment mechanism in which the recipient itself has to come and trigger the payment transaction.

1. Ocyticus and TokenggAVAX contracts inherits BaseAbstract but do not initializes the version parameter. Ideally the version should be passed in constructors/initializers so that its initialization does not get missed.

2. In the Vault contract, the transfer of funds is only allowed to network contracts, since Vault is non-upgradeable and intended to be future proof it should allow transfer of funds to any address.

3. Protocol docs for Ocyticus contract only mentions about pausing rights but unpausing is also possible.

4. In ERC20Upgradeable _disableInitializers should be invoked in the constructor, the same thing is also recommended by OpenZeppelin.

5. Many contracts redundantly inherit already inherited contracts. This makes the code less readable.

   Contract	Redundant Inheritance	Already Inherited In
   ERC4626Upgradeable	Initializable	ERC20Upgradeable
   TokenggAVAX	Initializable	ERC4626Upgradeable, BaseUpgradeable

6. The onlyGuardian modifier in ProtocolDAO.initialize function can be omitted as the function do not take any input values and the initialization is time independent.

7. onlyRegisteredNetworkContract modifier has different meaning and context in different contracts. In Storage it includes registered contracts and guardian while in BaseAbstract it only includes registered contracts.

8. In Staking contract, for increaseAVAXAssignedHighWater and resetAVAXAssignedHighWater onlySpecificRegisteredContract modifier should be used instead of onlyRegisteredNetworkContract modifer. As both the functions are only invoked by a single contract (MinipoolManager and ClaimNodeOp respectively).

9. MinipoolManager - incorrect comments. As per the convention of the overall protocol 2% should be 0.02 ether.

   • L35
   • L194

10. MinipoolManager - typo in @notice of canClaimAndInitiateStaking.

11. The protocol docs mentions that the contracts do not have any storage variables as they follow the Storage Registration Technique for upgradeability. But the contracts do have some storage variables as they inherit Base, ReentrancyGuard contracts.

12. ERC4626Upgradeable - a comment section mentions IMMUTABLES while there are no immutable variables in the contract.

13. In Vault contract no getter function is present to read allowedTokens mapping.

14. TokenggAVAX.sol imports BaseUpgradeable.sol using statement import "../BaseUpgradeable.sol"; which is not the recommended way for importing other solidity files as per the Solidity docs. This form is not recommended for use, because it unpredictably pollutes the namespace. The ideal way should be:

    import {BaseUpgradeable} from "../BaseUpgradeable.sol";

    Same is the case with ClaimNodeOp.sol, ClaimProtocolDAO.sol and some other contract files.

15. Unnecessary imports present:

    Contract File	Imported Object
    TokenggAVAX.sol	ERC20Upgradeable
    ClaimNodeOp.sol	MinipoolManager, TokenGGP
    MinipoolManager.sol	TokenGGP
    ProtocolDAO.sol	TokenGGP
    Vault.sol	TokenGGP, WAVAX

16. Vault contract contains unnecessary errors which are never used - InvalidNetworkContract, TokenTransferFailed and VaultTokenWithdrawalFailed.

17. Basic input sanitation for access protected functions should be done. Sometimes these functions are triggered by offchain automated softwares (scripts) which can possibly misbehave and pass null values (0). These null values can cause significant damage to the protocol and its users.

18. The codebase still has TODO tags. It is recommended to resolve all the TODO tags before deploying the contracts on mainnet.

1. In ERC20Upgradeable contract INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR can be made immutable.

   	uint256 internal INITIAL_CHAIN_ID;
       bytes32 internal INITIAL_DOMAIN_SEPARATOR;

2. In Storage.sol at L63 msg.sender should be used instead of reading newGuardian from storage.

   	if (msg.sender != newGuardian) {
   		revert InvalidGuardianConfirmation();
   	}
   	// ...
   	guardian = newGuardian;

3. MultisigManager - at L74 deleteBool should be used as the delete keyword uses less gas.

   74		setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), false);

4. ProtocolDAO - at L74 deleteBool should be used as the delete keyword uses less gas.

   74		setBool(keccak256(abi.encodePacked("contract.paused", contractName)), false);

5. In MultisigManager.requireNextActiveMultisig L82 can be omitted and addr can be declared in return datatypes.

   function requireNextActiveMultisig() external view returns (address) {
   	uint256 total = getUint(keccak256("multisig.count"));
   	address addr;       // L82
   	bool enabled;
   	for (uint256 i = 0; i < total; i++) {
   		(addr, enabled) = getMultisig(i);
   		if (enabled) {
   			return addr;
   		}
   	}
   	revert NoEnabledMultisigFound();
   }

6. In Vault.withdrawAVAX the second if condition (L71-L73) can be omitted. As the condition is already validated by solidity's underflow protection. Same is the case in transferAVAX, withdrawToken, transferToken functions.

7. In Vault.removeAllowedToken delete should be used as the delete keyword uses less gas.

   209		allowedTokens[tokenAddress] = false;

8. In Vault.withdrawToken at L157 there is no need to create new tokenContract variable in memory as tokenAddress is already of type ERC20.

   157		ERC20 tokenContract = ERC20(tokenAddress);

9. In Vault contract onlyRegisteredNetworkContract modifier can be removed from withdraw functions as the deposit balance of caller is validated.

10. In Staking contract increaseGGPStake function can be omitted and the needed statements can be written directly in the _stakeGGP function. The increaseGGPStake is only called once.

    function increaseGGPStake(address stakerAddr, uint256 amount) internal {
    	int256 stakerIndex = requireValidStaker(stakerAddr);
    	addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);
    }

11. MinipoolManager - In onlyOwner modifier msg.sender should be returned instead of memory variable.

    function onlyOwner(int256 minipoolIndex) private view returns (address) {
    	address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")));
    	if (msg.sender != owner) {
    		revert OnlyOwner();
    	}
    	return owner;
    }

12. MinipoolManager - In requireValidStateTransition function there is no need for the last else block.

    function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
    	// ...
    	bool isValid;
    	if (currentStatus == MinipoolStatus.Prelaunch) {
    		isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
    	} else if (currentStatus == MinipoolStatus.Launched) {
    	} else if {...}
    	else {
    		isValid = false;
    	}
        // ...
    }

13. Since Vault and TokenGGP contracts are non-upgradeable contracts, their addresses can be stored in immutable variables in other contracts instead of reading those addresses from the Storage contract.

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

    TokenGGP ggpToken = TokenGGP(getContractAddress("TokenGGP"));

14. In ClaimNodeOp.calculateAndDistributeRewards the rewardsPool.getRewardsCycleCount() value should be cached.

    	if (rewardsPool.getRewardsCycleCount() == 0) {
    		revert RewardsCycleNotStarted();
    	}
    
    	if (staking.getLastRewardsCycleCompleted(stakerAddr) == rewardsPool.getRewardsCycleCount()) {
    		revert RewardsAlreadyDistributedToStaker(stakerAddr);
    	}
    	staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());

15. In TokenggAVAX.depositFromStaking the msg.value should be used instead of totalAmt.

    function depositFromStaking(uint256 baseAmt, uint256 rewardAmt) public ... {
    	uint256 totalAmt = msg.value;
    	if (totalAmt != (baseAmt + rewardAmt) || baseAmt > stakingTotalAssets) {
    		revert InvalidStakingDeposit();
    	}
        // ...
    	IWAVAX(address(asset)).deposit{value: totalAmt}();
    }