ProtocolDAO: upgradeExistingContract deletes the new contract address making protocol inoperable

[code423n4]

Lines of code

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

Vulnerability details

Impact

The ProtocolDAO.upgradeExistingContract function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L216) allows to upgrade an existing contract in the protocol. E.g. it can be used to upgrade the Staking contract.

The function takes newAddr, newName and existingAddr as arguments.

It only really makes sense to upgrade an existing contract to a new contract that has the same name as the old contract. E.g. the staking contract must be called Staking before and after the upgrade. This is because other contracts look up the contract address by the contract's name:

Staking(getContractAddress("Staking"))

However the ProtocolDAO.upgradeExistingContract function calls registerContract first and then unregisterContract. Because both the new and the old contract have the same name, the contract.address entry of the new contract gets deleted again when unregisterContract is called.

Proof of Concept

1. ProtocolDAO.upgradeExistingContract is called to upgrade the Staking contract with these arguments: newAddr=0x02, newName="Staking", existingAddr=0x01.
2. The call to registerContract works fine
3. However after that, unregisterContract(0x01) is called which executes this line: deleteAddress(keccak256(abi.encodePacked("contract.address", name)));. This deletes the contract.address entry for the Staking contract. So other contracts in the protocol cannot find the Staking contract which makes the protocol inoperable

Tools Used

VSCode

Recommended Mitigation Steps

Call unregisterContract first and then registerContract such that the ProtocolDAO.upgradeExistingContract looks like this:

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