Vault's Owners can deploy fake PrizePools & TwabControllers contracts that can contain logic to cheat on users and alter the balances to get away with all the Prizes & potentially steal all the users underlying assets

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/VaultFactory.sol#L55-L86 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L258-L260

Vulnerability details

Impact

• Vault owners can alter the asset's balances and get away with all the Prizes & potentially all the users' deposits could be stolen.

Proof of Concept

• Vaults can be deployed permissionless by anybody, and all the deployed vaults through the factory will be registered as valid vaults.

  • If a Vault is registered in the VaultFactory, normal users might believe that is safe to interact with those Vaults.

• The problem is that the current process to create new Vaults allows the Vault creators (vault owners) to set the addresses for the PrizePool & TwabController contracts, malicious owners could take advantage of this and deploy fake contracts replicating the function's signatures of the original PrizePool & TwabController but implementing malicious code that can cheat the users and make the malicious owner to be able to get away with the user's deposits.

• Since the documentation states that the PrizePool & TwabController will be a singleton contract per chain, there is not really any benefit to allow the Vault Deployers to set at will the addresses of these two contracts for new vaults, on the other hand, allowing vault deployers to set these two addresses opens up a vector of attack that can cause users to loose their assets.

• It should be the responsibility of the protocol to ensure users' safeness by enforcing that all the deployed vaults use the real PrizePool & TwabController contracts, in this way, all the vaults that are registered on the VaultFactory are not susceptible to vault owners abusing and setting fake contracts as the PrizePool & TwabController.

Tools Used

Manual Audit

Recommended Mitigation Steps

• Since the TwabController and PrizePool are by design a singleton contract per chain, instead of allowing the vault's owners to set any address for these two contracts, those contract's addresses should be set as a contract variable in the VaultFactory and forwarded to each new Vault.

  • In this way, the protocol is forcing that all the vaults on the same chain will share the same PrizePool and TwabController contracts
  • By doing it in this way, the protocol is protecting the vault's users from being cheated by the vault's owners.

• Make the below code adjustments on the VaultFactory contract:

  
  contract VaultFactory {

• address private twabController_;

• address private prizePool_;

  ... ... ...

• constructor(address _twabController, address _prizePool) {

• twabController_ = _twabController;

• prizePool_ = _prizePool;

• }

  function deployVault( IERC20 _asset, string memory _name, string memory _symbol,

• TwabController _twabController,

• IERC4626 _yieldVault,

• PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint256 _yieldFeePercentage, address _owner ) external returns (address) { Vault _vault = new Vault( _asset, _name, _symbol,

• _twabController,

• _yieldVault,

• _prizePool,

• twabController_,

• prizePool_,

  _claimer, _yieldFeeRecipient, _yieldFeePercentage, _owner );

  ...

  }

}

- It is up to the protocol's decision whether they'd like to implement some functions to allow them to update the addresses of the TwapController and PrizePools in case those contracts are redeployed, and updated, but this means that the `VaultFactory` will need to introduce an owner.
  - This should be discussed by the protocol if this is something they'd like to do.

- If the protocol decides not to implement functions to update the TwapController and PrizePools addresses, then those variables could be set as `immutable`.

- Despite the final decision the protocol takes about implementing functions to update the TwapController and PrizePools contracts, the main suggested change needs to be implemented. Allowing vault's creators to set arbitrary addresses for core contracts can lead to undesired outcomes where users can even lose all their deposits

## Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #324

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory