GoGoPool contest - mookimgo's results

Lines of code

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

Vulnerability details

Impact

storage "contract.address"+name is wrongly set to zero when guardian call upgradeExistingContract with the same name

Proof of Concept

It's common that a contract will get updated with the same name, thus calling upgradeExistingContract with the same (newName is the same as the old name).

In this upgradeExistingContract function, it will first call registerContract function, which writes the storage setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);, but afterwards, it calls unregisterContract, which clears the storage: deleteAddress(keccak256(abi.encodePacked("contract.address", name)));.

So after this call, the contract.address+name is wrongly cleared, which makes getContractAddress fails in https://github.com/code-423n4/2022-12-gogopool/blob/1c30b320b7105e57c92232408bc795b6d2dfa208/contracts/contract/BaseAbstract.sol#L90 :

		if (contractAddress == address(0x0)) {
			revert ContractNotFound();
		}

This further makes all functionalities relying on getContractAddress fail

For example, after upgrade Staking contract, isEligible function in https://github.com/code-423n4/2022-12-gogopool/blob/1c30b320b7105e57c92232408bc795b6d2dfa208/contracts/contract/ClaimNodeOp.sol#L47 will fail.

Tools Used

reading code

Recommended Mitigation Steps

change to unregister first.

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