upgradeExistingContract will fail to set the new address for upgrades that keep the same name

[code423n4]

Lines of code

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

Vulnerability details

The upgradeExistingContract function present in the ProtocolDAO can be used by the guardian of the protocol to upgrade a contract by providing a new implementation address. Under the hood, state is kept in the Storage contract.

upgradeExistingContract implementation will call registerContract and unregisterContract

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

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

registerContract updates the storage for the new address:

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

function registerContract(address addr, string memory name) public onlyGuardian {
	setBool(keccak256(abi.encodePacked("contract.exists", addr)), true);
	setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
	setString(keccak256(abi.encodePacked("contract.name", addr)), name);
}

And unregisterContract will clear the storage associated with the previous address:

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

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

Since the unregisterContract call happens after the registerContract function, if the upgrade is done using the same contract name, then this will cause the address of the contract to be empty, since it is defined first during the registerContract call but immediately deleted in the call to unregisterContract. This operation should be expected, since throughout the codebase multiple contracts refer to other contracts by getting their addresses indexed by their names (using BaseAbstract.getContractAddress(name)).

Impact

Upgrading a contract with the same name will result in the contract's address being zero/empty (address(0)).

Other contracts that refer to the upgraded contract by name will fail, as they will try to call functions on the zero address, likely preventing some parts of the protocol from being executed.

PoC

The following test reproduces the issue. The "Staking" contract is first deployed and registered, and later upgraded to a second deployment by calling upgradeExistingContract. After the upgrade, the address for the "Staking" is address(0).

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	function setUp() public override {
		super.setUp();
	}
	
	function test_ProtocolDAO_upgradeExistingContract_FailsIfSameName() public {
		string memory name = "Staking";
		address firstDeploy = randAddress();

		vm.startPrank(guardian);

		// Staking is registered for the first deploy
		dao.registerContract(firstDeploy, name);

		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", name))), firstDeploy);

		// Now Staking is redeployed
		address secondDeploy = randAddress();

		dao.upgradeExistingContract(secondDeploy, name, firstDeploy);

		// Instead of pointing to the new deploy address, the address for the Staking is zeroed
		assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", name))), address(0));

		vm.stopPrank();
	}
}

Recommendation

This can be easily fixed by swapping the order of the actions, first unregistering the existing contract before registering the new one.

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