Lines of code

Vulnerability details

Impact

There is no way to recover from error state

Proof of Concept

To address report M-3, in PR,

<https://github.com/multisig-labs/gogopool/pull/20/files&gt;

The finishFailedMinipoolByMultisig method removed, while this does not block user from withdraw the fund in the error state in the current implementation.

I think the protocol is aware of this issue as shown in the open TODO:

// TODO how to handle error when Rialto is issuing validation tx? error status? or set to withdrawable with an error note or something?
function requireValidStateTransition(int256 index, DelegationNodeStatus to) private view {

First of all, when the staking error is recorded, the node id that maps to the staker will not able to be used.

According to <https://docs.avax.network/nodes/maintain/node-backup-and-restore#nodeid&gt;

Node id is very deterministic

> NodeID is a unique identifier that differentiates your node from all the other peers on the network. It’s a string formatted like NodeID-5mb46qkSBj81k9g9e4VFjGGSbaaSLFRzD. You can look up the technical background of how the NodeID is constructed here. In essence, NodeID is defined by two files:

• staker.crt
• staker.key

According to <https://docs.avax.network/apis/avalanchego/apis/p-chain#platformadddelegator&gt;

• Once you issue the transaction to add a node as a delegator, there is no way to change the parameters. You can’t remove a stake early or change the stake amount, node ID, or reward address.

Basically, the cost of changing the NodeID is very high. Suppose later the staker error is resolved, the same node id still cannot be used.

Another issue is that when the error is recorded, the corresponding multisig wallet is not disabled.

/// @notice Disabling a registered multisig
	/// @param addr Address of the multisig that is being disabled
	/// @dev this will prevent the multisig from completing validations. The minipool will need to be manually reassigned to a new multisig
	function disableMultisig(address addr) external guardianOrSpecificRegisteredContract("Ocyticus", msg.sender) {
		int256 multisigIndex = getIndexOf(addr);
		if (multisigIndex == -1) {
			revert MultisigNotFound();
		}

		setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), false);
		emit DisabledMultisig(addr, msg.sender);
	}

The issue is that once the staker goes to error state, and when the startRewardsCycle is called on RewardsManager.sol before the disableMultisig function is called, the address that is in error state can still get rewards

If the MultisigManager, once the wallet is registered, there is no way to unregister the wallet, the admin can only enable or disable the wallet.

As the comment suggest, the protocol does not expect a lot of wallet address.

/// @notice Gets the next registered and enabled Multisig, revert if none found
	/// @dev There will never be more than 10 total multisigs. If we grow beyond that we will redesign this contract.
	function requireNextActiveMultisig() external view returns (address) {

But moving the staker into error and not able to recover implies that 1 spot out of 10 total multisigs is taken forever.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol recover from error state to finish state once the staker claim the fund or when moving to error state, pushing the fund to the staker and change to the finish the state.

---

The text was updated successfully, but these errors were encountered:

All reactions