ProtocolDAO contract cannot be updated

[code423n4]

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

[GalloDaSballo]

Looks off because I got a ton of report of the function being wrong, but this is the only one mentioning a revert

[emersoncloud]

This is a special case where we delete the address of the ProtocolDAO and then try to make another change as the ProtocolDAO which fails.

[GalloDaSballo]

I believe it makes sense to group under #742

[GalloDaSballo]

I will award full merit despite this being an instance, because it's a specific instance that does make sense

[c4-judge]

GalloDaSballo marked the issue as duplicate of #742

[c4-judge]

GalloDaSballo marked the issue as satisfactory