Certain issues with `VaultDeployer.deployVault` and the resulting deployed vault contract.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/core/VaultDeployer.sol#L26-L28

Vulnerability details

Impact

1. The VaultDeployer.deployVault() function misses onlyVaultController modifier (if it is expected that only VaultController should be allowed to call)

2. It passes hardcoded msg.sender (EOA) for the param: _controllerAddress which is defined in comments as: 'Address of the VaultController'. This might DOS the vault functionality.

3. The deployVault function should check to see if the _id is already in use. This could be done by checking if there is already a vault with the same _id.

Proof of Concept

The deployVault is marked as external and allows anyone to access the function.

  function deployVault(uint96 _id, address _minter) external returns (IVault _vault) {
    _vault = IVault(new Vault(_id, _minter, msg.sender, CVX, CRV));
  }

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/core/VaultDeployer.sol#L26-L28

The msg.sender passed in place of controller will allow the creation of vault, but most functions will be DOS-ed and throw errors on calling since ethereum EOA accounts don't have functions/storage variables.

Further, the deployVault function should check to see if the _id is already in use. This could be done by checking if there is already a vault with the same id.

Tools Used

Manual

Recommended Mitigation Steps

Re-check if the function works as it is expected to.

I believe it is meant to accept a verified controller address, considering we want to allow creation of contract outside of the VaultController.

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as duplicate of #173

[c4-judge]

dmvt marked the issue as selected for report

[Minh-Trng]

This submission fails to provide any concrete impact, and to the best of my knowledge there are no issues from leaving the function open:

The VaultDeployer.deployVault() function misses onlyVaultController modifier (if it is expected that only VaultController should be allowed to call)

while the statement is correct, the warden does not describe any actual impacts from leaving this unprotected. The only one I can think of is the Re-Org related submission: https://github.com/code-423n4/2023-07-amphora-findings/issues/233 (solved by using create2)

It passes hardcoded msg.sender (EOA) for the param: _controllerAddress which is defined in comments as: 'Address of the VaultController'. This might DOS the vault functionality.

Why would anyone call this from an EOA in the first place if they dont know what they are doing? It is correct that almost all functions will fail to work if called from an EOA, so there is no merit in doing that.

The deployVault function should check to see if the _id is already in use. This could be done by checking if there is already a vault with the same _id.

The VaultController keeps track of the addresses belonging to an Id. While it is possible for multiple Vaults with the same Id to exist, the VaultController knows which of them was deployed the "right" way and only will interact with that one. There is no way to exploit this

  function mintVault() public override whenNotPaused returns (address _vaultAddress) {
    // increment  minted vaults
    vaultsMinted += 1;
    // mint the vault itself, deploying the contract
    _vaultAddress = _createVault(vaultsMinted, _msgSender());
    // add the vault to our system
    vaultIdVaultAddress[vaultsMinted] = _vaultAddress;

    //push new vault ID onto mapping
    walletVaultIDs[_msgSender()].push(vaultsMinted);

    // emit the event
    emit NewVault(_vaultAddress, vaultsMinted, _msgSender());
  }

[dmvt]

On review I mostly agree with @Minh-Trng. None of the wardens reporting this issue showed how it would cause the protocol to malfunction or lead to a loss of funds. That makes this a comment related issue. Downgrading to QA.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b

[c4-judge]

dmvt marked the issue as not selected for report