upgradeExistingContract will fail to register the new address when newName == oldName.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.
when the new name of the contract is the same as the old name of a contract (which is necessary to move funding from old contracts to upgraded ones), the function upgradeExistingContract() will fail to register the new address of the contract and in particular, getContractAddress(newName) will return zero after the upgrade. The whole system will not work after that since modifiers guardianOrSpecificRegisteredContract() and onlySpecificRegisteredContract() will always revert for this contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L217
The main problem of upgradeExistingContract() is that it performs registerContract() first and then performs unregisterContract(). The order should be reversed. Because of this, the following statement of registerContract()

setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);

is overwritten by the following statement of unregisteredContract():

deleteAddress(keccak256(abi.encodePacked("contract.address", name)));

as a result, the registration of the address of the contract under the name is lost when oldName == newName.
Moreover, getContractAddress(newName) will return zero after the upgrade. The whole system might fail after that.

Tools Used

Remix

Recommended Mitigation Steps

The fix is simple, just exchange the order of registerContract() and unregisterContract() in function upgradeExistingContract():

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

[c4-judge]

GalloDaSballo marked the issue as duplicate of #742

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory