Popcorn contest - hashminer0725's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L669

Vulnerability details

Impact

The function is expected to revert only if the caller is neither the submitter nor the owner. But the current condition requires that the caller should be the submitter as well as the owner.

if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L669

Tools Used

Manual Review

Recommended Mitigation Steps

Should use && instead of ||. Like follwing.

if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

Judge has assessed an item in Issue #56 as 2 risk. The relevant finding follows:

2. Attribute values of fees_ could exceed 1e18 when initializing even if the proposedFees is checked in proposeFees() function. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L88

   function initialize( IERC20 asset_, IERC4626 adapter_, VaultFees calldata fees_, address feeRecipient_, address owner ) external initializer { ... fees = fees_; function proposeFees(VaultFees calldata newFees) external onlyOwner { if ( newFees.deposit >= 1e18 || newFees.withdrawal >= 1e18 || newFees.management >= 1e18 || newFees.performance >= 1e18 ) revert InvalidVaultFees();

   proposedFees = newFees;
   

initialize() function could have a check if the attribute values of fees_ are less than 1e18.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

Vulnerability details

Impact

proposedFees is set to a default struct which all attribute fees are 0 when deploying the contract. And changeFees() function can be called by anyone. proposedFees is updated when the owner proposes the fees by calling proposeFees() function. So if the owner has not proposed the fees yet after deploying the contract, the attacker can change all fees to 0 by calling changeFees() function.

function changeFees() external {
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
}

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

Tools Used

Manual Review

Recommended Mitigation Steps

changeFees() function can have onlyOwner modifier or have a check if proposedFeeTime is not 0.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L372-L381

Vulnerability details

Impact

proposedFees of vaults are set to a default struct which all attribute fees are 0 when deploying the vault contracts. If the creator has not proposed the fees yet after deploying the vaults, the attacker can change the fees of vaults to 0 by calling changeVaultFees() function.

function changeVaultFees(address[] calldata vaults) external {
    uint8 len = uint8(vaults.length);
    for (uint8 i = 0; i < len; i++) {
      (bool success, bytes memory returnData) = adminProxy.execute(
        vaults[i],
        abi.encodeWithSelector(IVault.changeFees.selector)
      );
      if (!success) revert UnderlyingError(returnData);
    }
}

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L372-L381

Tools Used

Manual Review

Recommended Mitigation Steps

It can have a check if the caller is the creator of the vault to be updated.

for (uint8 i = 0; i < len; i++) {
      _verifyCreator(vaults[i]);
      (bool success, bytes memory returnData) = adminProxy.execute(
        vaults[i],
        abi.encodeWithSelector(IVault.changeFees.selector)
      );
      if (!success) revert UnderlyingError(returnData);
}

1. success could be defined in the first if condition

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/CloneFactory.sol#L42

    bool success = true;
    if (template.requiresInitData) (success, ) = clone.call(data);

    if (!success) revert DeploymentInitFailed();

success could be defined in the first if condition. If template.requiresInitData is false, checking !success is unnecessary.

2. Attribute values of fees_ could exceed 1e18 when initializing even if the proposedFees is checked in proposeFees() function.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L88

    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,
        address feeRecipient_,
        address owner
    ) external initializer {
        ...
        fees = fees_;

    function proposeFees(VaultFees calldata newFees) external onlyOwner {
        if (
            newFees.deposit >= 1e18 ||
            newFees.withdrawal >= 1e18 ||
            newFees.management >= 1e18 ||
            newFees.performance >= 1e18
        ) revert InvalidVaultFees();

        proposedFees = newFees;

initialize() function could have a check if the attribute values of fees_ are less than 1e18.

3. convertToShares() function is used twice.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L143 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L147

    uint256 feeShares = convertToShares(
        assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
    );

    shares = convertToShares(assets) - feeShares;

It could calculate the share amount for asset and then calculate feeShares and shares to be minted from the amount. Like doing in other functions.

4. assetsCheckpoint is never used.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L467

    uint256 public assetsCheckpoint;

The variable could be removed from the contract.

5. _registerCreatedVault() function doesn't need to have metadata parmeter.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L144

    function _registerCreatedVault(
      address vault,
      address staking,
      VaultMetadata memory metadata
    ) internal {
      metadata.vault = vault;
      metadata.staking = staking;
      metadata.creator = msg.sender;

metadata is set in the function again.

6. Some attributes could be removed from the struct.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IVaultRegistry.sol#L16-L21

    /// @notice OPTIONAL - If the asset is an Lp Token these are its underlying assets
    address[8] swapTokenAddresses;
    /// @notice OPTIONAL - If the asset is an Lp Token its the pool address
    address swapAddress;
    /// @notice OPTIONAL - If the asset is an Lp Token this is the identifier of the exchange (1 = curve)
    uint256 exchange;

swapTokenAddresses, swapAddress, and exchange are never used from the contracts.

7. newCooldown can include 1 day even if 1 day is not allowed in VaultController.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L502

    if (newCooldown >= 1 days) revert InvalidHarvestCooldown(newCooldown);

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L793

    if (newCooldown > 1 days) revert InvalidHarvestCooldown(newCooldown);

8. NoteSubmitter() is never used.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L226

    error NotSubmitter(address submitter);

9. NoFee(IERC20 token) is never used.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L200

    error NoFee(IERC20 token);

10. setPermissions() function is only reverted if both newPermissions[i].endorsed and newPermissions[i].rejected are true. But it also should be reverted if both are false.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/PermissionRegistry.sol#L43

    if (newPermissions[i].endorsed && newPermissions[i].rejected) revert Mismatch();

11. accrued could be removed from the Fee struct.

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/interfaces/IMultiRewardEscrow.sol#L26

24    struct Fee {
25      /// @notice Accrued fee amount
26      uint256 accrued;
27      /// @notice Fee percentage in 1e18 for 100% (1 BPS = 1e14)
28      uint256 feePerc;
29    }

It's never used from the contracts.