ProtocolDAO::upgradeExistingContract is not working as intended and can have bad consequences

[code423n4]

Lines of code

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

Vulnerability details

The contracts used in GoGoPool protocol are using fixed names when interacting with each other. For example:

Vault vault = Vault(getContractAddress("Vault"));
Staking staking = Staking(getContractAddress("Staking"));
RewardsPool rewardsPool = RewardsPool(getContractAddress("RewardsPool"));
TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
...

The function upgradeExistingContract if used to upgrade an existing contract name with a new address will result in getContractAddress function to return zero address for the upgraded contract address.

If used with a new name then getContractAddress will return the supplied address but as already stated the protocol uses fixed names.

Impact

If an upgrade like this happen funds can be lost for example:

When distributing rewards

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

if (daoClaimContractAllotment > 0) {
    emit ProtocolDAORewardsTransfered(daoClaimContractAllotment);
    vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment);
}

funds can be lost if the upgrade happened for "Vault" when vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment); is called the function will not revert but also the token amount will be lost

Or for example in claimAndRestake function:

if (claimAmt > 0) {
    vault.withdrawToken(msg.sender, ggp, claimAmt);
}

again vault.withdrawToken(msg.sender, ggp, claimAmt); will not work as expected.

Proof of Concept

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

Add the following test to ProtocolDAO.t.sol unit test file:

function testUpgradeExistingContract_POC1() public {
    address addr = randAddress();

    address existingAddr = randAddress();
    string memory existingName = "existingName";

    bytes32 testKey = "testKey";

    vm.prank(guardian);
    dao.registerContract(existingAddr, existingName);
    assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", existingAddr))), true);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", existingName))), existingAddr);
    assertEq(store.getString(keccak256(abi.encodePacked("contract.name", existingAddr))), existingName);

    vm.prank(guardian);
    dao.upgradeExistingContract(addr, existingName, existingAddr);
    assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", addr))), true);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", existingName))), addr);
}

run with forge test -m testUpgradeExistingContract_POC1 the test will fail when comapring the expected address with the real return after upgrading an existing contract name:

[FAIL. Reason: Assertion failed.] testUpgradeExistingContract_POC1() (gas: 159000)
Logs:
  Error: a == b not satisfied [address]
    Expected: 0x934eb1f9d0f59695050f761dc64e443e5030a569
      Actual: 0x0000000000000000000000000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Invert the order of operation as stated in the function comment header

/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name
function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
-	registerContract(newAddr, newName);
-	unregisterContract(existingAddr);
+   unregisterContract(existingAddr);
+   registerContract(newAddr, newName);
	}

[GalloDaSballo]

I really liked the logs of the reverting test with error

[c4-judge]

GalloDaSballo marked the issue as duplicate of #742

[c4-judge]

GalloDaSballo marked the issue as satisfactory