GoGoPool contest - Czar102's results

Lines of code

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

Vulnerability details

Impact

A wrong implementation of the upgradeExistingContract in the ProtocolDAO contract may corrupt storage.

This happens when wither new and existing addresses are eual or when the old name and new names are equal.

Such storage corrption may have disastrous effects, from including deprecated contracts to not including a contract that is to be within the mapping the Storage contract offers.

Proof of Concept

The following function

	/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name
	/// @param newAddr Address of the new contract
	/// @param newName Name of the new contract
	/// @param existingAddr Address of the existing contract to be deleted
	function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

contains a bug: a contract is registered firstly, then removed.

If the address of the contract is the same and the name is updated, the new name is mapped to the address and the address is mapped to the new name. Then, the contract is unregistered, so what was added is deleted. This leaves "contract.exists" false for the address, the past name is still mapped for the contract address but the address is not mapped to any name. Storage corruption.

If the address is new but the name is the same, registering adds the contract and adds mappings to the name and backwards. The deletion will unregister the old address from existance, but also delete data from the mapping of names to addresses since the unregisterContract will read the same name for the old contract. Again, the storage is left corrupted.

Tools Used

Manual analysis

Recommended Mitigation Steps

Firstly delete the old existence from the storage, then add (register, then unregister). Also, make sure that the registration happens only if a token isn't already registered not to corrupt storage.