code-423n4 / 2022-12-gogopool-findings

1 stars 0 forks source link

Upgraded Q -> 2 from #867 [1675460709593] #899

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #867 as 2 risk. The relevant finding follows:

L-01, ProtocolDAO.sol lines 209 - 216:
upgradeExistingContract mistakenly removes the address value of the new contract if the new contract’s name is the same as the old one. This can be easily fixed with unregistering first and then registering with can be done manually by the guardian if he is careful.
Also, if the name of the contract is changing, the value of the previous contract should be moved to the new one. However, since the name of the contracts is hardcoded into the other ones for handling permissions, I assume the name of the contract would stay the same or a whole new set of contract should be deployed together. PoC is included below: function testUpgradeExistingContractSameName() public { address addr = randAddress(); string memory name = "existingName";

    address existingAddr = randAddress();
    string memory existingName = "existingName";

    vm.prank(guardian);
    dao.registerContract(existingAddr, existingName);
    assertEq(store.getBool(keccak256(abi.encodePacked("contract.exists", existingAddr))), true);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", existingName))), existingAddr);
    assertEq(store.getString(keccak256(abi.encodePacked("contract.name", existingAddr))), existingName);

    vm.prank(guardian);
    dao.upgradeExistingContract(addr, name, existingAddr);
    assertEq(store.getAddress(keccak256(abi.encodePacked("contract.address", name))), address(0));
}
c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #742

c4-judge commented 1 year ago

GalloDaSballo marked the issue as satisfactory