There may be errors when ProtocolDAO upgrades a contract with the same name

[code423n4]

Lines of code

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

Vulnerability details

Impact

• The ProtocolDAO contract cannot be upgraded without renaming.
• If the ProtocolDAO is upgraded through upgradeExistingContract() without renaming and accidentally passes in the existingAddr as the newAddr, the protocol will be in error and never be upgraded again.
• All other contracts that upgrade through upgradeExistingContract() without changing their name will cause error.

Proof of Concept

Since there are many direct name references between contracts, a contract can't change its name if it is to be upgraded separately.
However, the current implementatin of ProtocolDAO may cause errors if a contract upgrade without renaming.

Suppose ProtocolDAO is to be upgraded separately, from addr1 to addr2.

1. If upgrade by calling upgradeExistingContract(addr2, "ProtocolDAO", addr1): the storage will be in wrong state, because the address stored in keccak256(abi.encodePacked("contract.address", "ProtocolDAO")) will be deleted by unregisterContract(addr1).

function unregisterContract(address addr) public onlyGuardian {
  	string memory name = getContractName(addr);
  	deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
  //!! address deleted
  	deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
  	deleteString(keccak256(abi.encodePacked("contract.name", addr)));
  }

2. If upgrade by calling registerContract(addr2) and then unregisterContract(addr1): same error as case-1 above.
3. If upgrade by calling unregisterContract(addr1) and then registerContract(addr2): the second call will fail because the ProtocolDAO is no longer available (unregistered) after the first call, and the error will be unfixable because no contract can be upgraded anymore.
4. If upgrade by calling upgradeExistingContract(addr1, "ProtocolDAO", addr1) (accidentally provide addr1 as addr2): ProtocolDAO will be unavailable (unregistered), and the error will be unfixable because no contract can be upgraded anymore.

Any contract that upgrade alone (without renaming) through upgradeExistingContract will cause error.
Similar to case 1/2 above, upgradeExistingContract(addr2, name, addr1) will case the address stored in keccak256(abi.encodePacked("contract.address", name)) being deleted.

Tools Used

VS Code

Recommended Mitigation Steps

The ProtocolDAO contract itself should be prohibited from being unregistered directly through unregisterContract() (Prevent ProtocolDAO from being unregistered due to misoperations).
upgradeExistingContract() should determine the case of an upgrade without a name change (as this will be common) and handle the case correctly.

Sample code:

diff --git a/contracts/contract/ProtocolDAO.sol b/contracts/contract/ProtocolDAO.sol
index d0846f6..187bbf4 100644
--- a/contracts/contract/ProtocolDAO.sol
+++ b/contracts/contract/ProtocolDAO.sol
@@ -196,9 +196,16 @@ contract ProtocolDAO is Base {
        /// @notice Unregister a contract with Storage
        /// @param addr Contract address to unregister
        function unregisterContract(address addr) public onlyGuardian {
+        require(addr != address(this));
+        _unregisterContract(addr, "");
+       }
+
+       function _unregisterContract(address addr, string memory upgradeName) internal {
                string memory name = getContractName(addr);
                deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
-               deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
+        if (keccak256(abi.encodePacked((name))) != keccak256(abi.encodePacked((upgradeName)))) {
+            deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
+        }
                deleteString(keccak256(abi.encodePacked("contract.name", addr)));
        }

@@ -212,6 +219,6 @@ contract ProtocolDAO is Base {
                address existingAddr
        ) external onlyGuardian {
                registerContract(newAddr, newName);
-               unregisterContract(existingAddr);
+               _unregisterContract(existingAddr, newName);
        }
 }

[c4-judge]

GalloDaSballo marked the issue as duplicate of #742

[c4-judge]

GalloDaSballo marked the issue as satisfactory