Upgrading existing contract in storage can result in unwanted state

[code423n4]

Lines of code

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

Vulnerability details

Impact

If there is a need to upgrade an existing contract due to vulnerability or added functionality, upgradeExistingContract could be called to simplify unregistering and registering a new contract in one transaction. However, registering a new contract before unregistering the previous address can leave keccak256(abi.encodePacked("contract.address", name) without the correct value in addressStorage if newAddr and existingAddr's name in storage is equal to the function parameter newName. This would be the assumed case given the fact that the underlying contract address is being upgraded for that name so that other areas of the GoGoPool protocol can interact with the newly upgraded contract at that name which is hardcoded and used throughout the protocol.

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

## Proof of concept
For example, if the contract you was replacing was named "RewardsPool" with an existing address of 0x0000000000000000000000000000000000000001 and upgradingExistingContract is called with newName "rewardsPool" and address 0x0000000000000000000000000000000000000002 it would call registerContract setting the values required in storage correctly. Next, unregisterContract would be called with the existing address in which the contract.exists bool and contract.name string would be removed correctly. However, the contract.address field which is packed and hashed with the same name "RewardsPool" would be removed although that mapping in storage was already updated to the new address already in registerContract.

Tools Used

VSCode

Recommended Mitigation Steps

If unregisterContract(existingAddr); is called before registerContract(newAddr, newName); in upgradeExistingContract, this would resolve that issue where the addressStorage mapped value is set and stays changed to newAddr.

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 marked the issue as satisfactory