Insulfficient validation of input parameters in vaultfactory.sol:: deployvault:

[code423n4]

Lines of code

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

Vulnerability details

Impact

Malicious vaultowner can depoy vault using malicious twabcontroller, yieldvault and prizepool contracts that can put users' funds at risk.

Proof of Concept

In the contract vaultfactory.sol, the function deployvault is used to deploy new vaults and then push it to the allvaults array. The addresses of the twabcontroller, yieldvault and prizepool were passed as input in the function but were not validated before the vault was deployed, in the cunstructor() of the vaults.sol contract, the if statement only checks that the addresses are not the zero address, but does not ensure that the addresses are the right ones.

 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,
      _claimer,
      _yieldFeeRecipient,
      _yieldFeePercentage,
      _owner
    );

    allVaults.push(_vault);
    deployedVaults[_vault] = true;

    emit NewFactoryVault(_vault, VaultFactory(address(this)));

    return address(_vault);
  }

Tools Used

Manual review

Recommended Mitigation Steps

ensure proper validation since these parameters are passed in manually which can possibly lead to some typos.

Assessed type

Invalid Validation

[Picodes]

Regrouping here all issues related to using the permissionless nature of the factory to create a malicious deployment, and abusive usage of owner privileges

[c4-judge]

Picodes marked the issue as primary issue

[asselstine]

This the point of V5: we are replacing protocol gatekeeping with curation by front-ends.

The responsibility of curation will rest on front-ends, who will curate which vaults they show their users.

[c4-sponsor]

asselstine marked the issue as sponsor acknowledged

[Picodes]

I'll keep this issue as a valid Medium to highlight the importance of properly checking deployments or trusting the front-end.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked issue #300 as primary and marked this issue as a duplicate of 300