GoGoPool contest - Saintcode_'s results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L80-L91 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L236

Vulnerability details

MultisigManager.sol function requireNextActiveMultisig() will always return the first enabled multisig address instead of the next one.

Impact

Only the first registered multisig address is always assigned to a pool, making the rest of the registered multisig addresses useless. And therefore less decentralized.

Proof of Concept

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L236 createMinipool() function on MiniPoolManager.sol calls multisigManager.requireNextActiveMultisig() to assign a new multisig to the current pool.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L80-L91 But the requireNextActiveMultisig() function on MultisigManager.sol always returns the first enabled multisig address, as it traverses all multisigs by index from 0 to count and returns the first enabled one instead of the next one:

for (uint256 i = 0; i < total; i++) {
			(addr, enabled) = getMultisig(i);
			if (enabled) {
				return addr;
			}
		}

Tools Used

vscode, Foundry

Recommended Mitigation Steps

If the protocol intends to get the next multisig address as the function name requireNextActiveMultisig() says, a good fix would be to keep track of the last assigned multisig address on MinipoolManager and start traversing all multi sig from that index instead of from position 0.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L17 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L27

Vulnerability details

It is not possible to change, unregister or replace a multisig address from the protocol, it is only possible to disable a multisig. What limits the protocol to the same 10 multisig addresses forever

Impact

Once the MULTISIG_LIMIT is reached, there is no way of upgrading a multisig or even unregistering it which makes the protocol bound to the same 10 addresses forever, therefore if there's a need to upgrade them, the MultisigManager would have to be redeployed with new logic to permit multisig replacement.

Recommended Mitigation Steps

Implement a way to unregister or change a current multisig by index, following registerMultisig() implementation but adding an index parameter. A possible implementation would be:

	function changeMultisig(address addr, uint256 index) external onlyGuardian {
		uint256 currentIndex = getUint(keccak256("multisig.count"));
		if (index > currentIndex ) {
			revert MultisigDoesNotExist();
		}

		setAddress(keccak256(abi.encodePacked("multisig.item", index, ".address")), addr);

		setUint(keccak256(abi.encodePacked("multisig.index", addr)), index + 1);
		emit ChangedMultisig(addr, msg.sender);
	}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L528-L533

Vulnerability details

When Multisig calls recordStakingError() to record that a staking error occurred while registering the node as a validator, the MinipoolCount for the owner of the minipool is not decreased. This makes the state of the protocol unstable since a user that goes from the MinipoolStatus.Error state to MinipoolStatus.Finished will still have a MinipoolCount of 1 without having any active pools. And a user that goes from MinipoolStatus.Error to MinipoolStatus.Prelaunch would have a MinipoolCount of 2 even though he only has one active pool.

This incorrect state of the protocol makes a user still eligible for the next cycle of rewards without having an active staking Minipool.

Functions _cancelMinipoolAndReturnFunds() and recordStakingEnd() call staking.decreaseMinipoolCount(owner) to decrease Minipoolcount when a pool is no longer staking: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L435-L437

Staking staking = Staking(getContractAddress("Staking"));
staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
staking.decreaseMinipoolCount(owner);

But neither recordStakingError() nor finishFailedMinipoolByMultisig() call that function to decrease Minipoolcount when a pool is no longer staking. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L509-L510

Staking staking = Staking(getContractAddress("Staking"));
staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);

Recommended Mitigation Steps

It is recommended to call staking.decreaseMinipoolCount(owner) on the recordStakingError() function right after decreasing AVAX assigned in order to maintain a correct count of active staking pools and protect the system against rewarding a user that's not longer staking.

Tools Used

vscode, Foundry