[Lines of code](https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Pool/PoolRegistry.sol#L393-L408) # Vulnerability details ## Proof of Concept PoolRegistry.createPoolRegistry() will call PoolRegistry._registerPool() which will make a check on the creator field for a pool. VenusPool memory venusPool = _poolByComptroller[comptroller]; require(venusPool.creator == address(0), "PoolRegistry: Pool already exists in the directory."); However, creator is never set, e.g. it will always be the zero address for a venus pool, and the require statement highlighted above has no impact. To override an existing pool, it's necessary to generate the same comptroller address (proxyAddress and comptrollerProxy) from an existing pool. Therefore, the following params need to be equal to the ones being used on an existing pool: beaconAddress, abi.encodeWithSelector(Comptroller.initialize.selector, maxLoopsLimit, accessControlManager. However, closeFactor, liquidationIncentive, minLiquidatableCollateral and priceOracle can be overridden. It is also possible to override the state variable _poolByComptroller[comptroller] which will be used to set the pool name and it will be used to to query pools data. ## Impact Perhaps overriding a pool with different params is not a major issue since PoolRegistry.createPoolRegistry() is permissioned. However, if we consider the case of an attacker taking over the the account(s) allowed to call PoolRegistry.createPoolRegistry(), then the attacker can replace pool params for existing pools to cause damage for honest users using those pools. Assuming the owner is a timelock multisig, then the likelihood is small but the impact is considerable. For example, the attacker can set the max uint256 value in liquidationIncentive, and pass zero for minLiquidatableCollateral to break liquidations. If an attacker takes control the account(s) able to call PoolRegistry.createPoolRegistry(), the project will have more issues than the ones described in this report, but this context of not setting the pool creator is an extra point of failure that can be fixed easily. # Tools Used Manual review. ## Recommended Mitigation Steps Since the following check for venusPool.creator is already being used: require(venusPool.creator == address(0), consider setting the creator in the same function, e.g. diff --git a/PoolRegistry.sol.orig b/PoolRegistry.sol --- a/PoolRegistry.sol.orig +++ b/PoolRegistry.sol @@ -394,6 +394,7 @@ contract PoolRegistry is Ownable2StepUpgradeable, AccessControlledV8, PoolRegist VenusPool memory venusPool = _poolByComptroller[comptroller]; require(venusPool.creator == address(0), "PoolRegistry: Pool already exists in the directory."); + venusPool.creator = msg.sender; _ensureValidName(name); _numberOfPools++; ## Assessed type Other --- The text was updated successfully, but these errors were encountered: All reactions

It is possible to override pool params in PoolRegistry.sol since the creator field is never set for a venus pool