Potential Failure to Handle Vault Creation Errors in mintVault Function

[code423n4]

Lines of code

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

Vulnerability details

Impact

The mintVault function allows users to create new vaults. However, it fails to handle potential errors that may arise during the vault creation process. This omission may lead to unexpected behavior and leave users uninformed about the status of the vault creation.

Proof of Concept

Within the mintVault function, after incrementing the vaultsMinted counter, the function proceeds to call the internal _createVault function to deploy a new vault. However, the return value of _createVault, which represents the address of the newly created vault, is not explicitly checked for validity.

  function _createVault(uint96 _id, address _minter) internal virtual returns (address _vault) {
    _vault = address(VAULT_DEPLOYER.deployVault(_id, _minter));
  }

Possible Consequences:

If the _createVault function encounters an error during deployment, such as running out of gas, encountering an exception, or failing due to an issue in the VAULT_DEPLOYER contract, it will return the null address address(0).

As the return value is not checked, the function continues execution, assuming the vault creation was successful, which could lead to inconsistencies and unexpected behavior in the application. Users will not be aware of the failure, as no error messages or exceptions are raised, potentially causing confusion and difficulty in diagnosing the issue.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, it is crucial to add a check to verify the return value of _createVault before proceeding with the rest of the mintVault function. If the return value is equal to address(0), the function should revert the transaction, signaling the failure to create the vault and providing a meaningful error message. This way, users will be properly informed in case of any issues during the vault creation process.

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

    require(_vaultAddress != address(0), "Vault creation failed");

    // 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());
}

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

If it run out of gas, it will revert

[c4-judge]

dmvt marked the issue as unsatisfactory: Overinflated severity

[dmvt]

As written the only impact is an unnecessary gas fee. This does not qualify as medium risk.

[Nabeel-javaid]

Hey, can you please have another look at this, cause if the return value isn't checked then the function mintVault will add a new vault which will actually be an address(0) (since if the deployment of new vault fails it will return address(0)) which will cause many issues.