GoGoPool contest - nogo's results

Lines of code

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

Vulnerability details

Impact

upgradeExistingContract is the method allowing an upgrade of registered contracts. By using the same name, it is impossible to update the address of the contract using the method as the name is overwritten.

The impact is that the contracts need to have a new name for each update. This will result in the update of smart contracts where the name is being used (for example in Staking.sol where "MinipoolManager" is having some privileged access). This will introduce an error-prone process that can lead to protocol disruption.

Proof of Concept

The method is updating the storage from Storage.sol by first adding the new implementation and removing the existing one.

If for example, the guardian wants to update the "MinipoolManager", the method will first register it using the same name as a key for the address and then remove it. This will result in the name being overwritten and the contract address being equal to 0x0.

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

To reproduce it, add this method in the ProtocolDAO.t.sol and run the test by using:

forge test --match-test "testUpgradeSameName" -vvvv

Tools Used

Visual Studio Code, Unit tests (Foundry)

Recommended Mitigation Steps

The simplest mitigation would be to add a check for name and newName equality if this method does not support the same name and specify that only the Guardian can do it (by calling unregisterContract and registerContract).

Otherwise, by first unregistering the contract, secondly registering the new one, and finally removing the bool, the final value in the storage will be the new address:

function upgradeExistingContract(
  address newAddr,
  string memory newName,
  address existingAddr
) external onlyGuardian {
  string memory existingName = getContractName(existingAddr);

  deleteAddress(keccak256(abi.encodePacked("contract.address", existingName)));
  deleteString(keccak256(abi.encodePacked("contract.name", existingAddr)));
  setBool(keccak256(abi.encodePacked("contract.exists", newAddr)), true);
  setAddress(keccak256(abi.encodePacked("contract.address", newName)), newAddr);
  setString(keccak256(abi.encodePacked("contract.name", newAddr)), newName);
  deleteBool(keccak256(abi.encodePacked("contract.exists", existingAddr)));
}

This echoes my next medium risk (ProtocolDAO contract cannot be updated).

Lines of code

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

Vulnerability details

Impact

The protocol has chosen a data separation upgrade mechanism which means that all the data is located in a smart contract not upgradable and the logic in upgradeable contracts. TheProtocolDAO contract which is part of the upgradable ones via the storage registration technique (as per documentation) is actually not upgradeable.

The impact is that the ProtocolDAO contract cannot be updated which means that, if any logic is added to the smart contract, the protocol will be stuck with it.

Proof of Concept

The modifier onlyRegisteredNetworkContract is limiting the execution of setting and deleting keys in Storage.sol.

Practically when updating the ProtocolDAO, the ProtocolDAO contract is unregistered which means that the following key keccak256(abi.encodePacked("contract.exists", addr))) is deleted and will revert when deleting the address due to the condition check on the key in the modifier.

// Storage.sol
modifier onlyRegisteredNetworkContract() {
	if (booleanStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))] == false && msg.sender != guardian) {
		revert InvalidOrOutdatedContract();
	}
	_;
}

// ProtocolDAO.sol
function registerContract(address addr, string memory name) public onlyGuardian {
	setBool(keccak256(abi.encodePacked("contract.exists", addr)), true);
	setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
	setString(keccak256(abi.encodePacked("contract.name", addr)), name);
}
function unregisterContract(address addr) public onlyGuardian {
	string memory name = getContractName(addr);
	deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
	deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
	deleteString(keccak256(abi.encodePacked("contract.name", addr)));
}
function upgradeExistingContract(
	address newAddr,
	string memory newName,
	address existingAddr
) external onlyGuardian {
	registerContract(newAddr, newName);
	unregisterContract(existingAddr);
}

To reproduce it, add this method in the ProtocolDAO.t.sol and run the test by using:

forge test --match-test "testUpgradeProtocolDao" -vvvv

Tools Used

Visual Studio Code, Unit tests (Foundry)

Recommended Mitigation Steps

By removing the boolean existence at the end, we are allowing the deletion of the address and the name of the contract without triggering the exception from onlyRegisteredNetworkContract.

function upgradeExistingContract(
  address newAddr,
  string memory newName,
  address existingAddr
) external onlyGuardian {
  string memory existingName = getContractName(existingAddr);

  deleteAddress(keccak256(abi.encodePacked("contract.address", existingName)));
  deleteString(keccak256(abi.encodePacked("contract.name", existingAddr)));
  setBool(keccak256(abi.encodePacked("contract.exists", newAddr)), true);
  setAddress(keccak256(abi.encodePacked("contract.address", newName)), newAddr);
  setString(keccak256(abi.encodePacked("contract.name", newAddr)), newName);
  deleteBool(keccak256(abi.encodePacked("contract.exists", existingAddr)));
}

Lines of code

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

Vulnerability details

Impact

requireNextActiveMultisig is giving the next available multisig in order to assign it to a minipool during creation. The way the method is constructed, only the first enabled multisig will ever be returned which will prevent other multisig from being used.

Proof of Concept

The for loop, which starts at index 0 until the multisig count, is returning the first enabled multisig:

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

As per the inline comment, a maximum of 10 multisigs will be added to the protocol. But if the first multisig is enabled, the nine other multisigs will never be used and the protocol will rely on the same multisigs. The protocol is rewarding equally all multisigs whereas only one will actually work. 2 issues can be raised here:

• if the multisig is compromised, the protocol will be compromised as well
• as the rewards mechanism is equal, the incentivization model is not fair, which can lead to a lack of trust in the protocol from the multisigs.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

The for loop should be changed to start at the last index used (stored in storage) and iterate until finding one multisig is enabled. This will ensure that all multisig added will be used.