Popcorn contest - DadeKuma's results

Lines of code

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

Vulnerability details

Impact

Users can withdraw from a Vault even if they don't have any shares, and steal vault assets until it has zero shares in the Adapter, due to a rounding error.

Proof of Concept

Let's suppose that there is a Vault with totalSupply = 100_000 and totalAssets = 100_000_000

Alice calls withdraw with 999 assets, with herself as the receiver, and owner. Let's suppose that there aren't any withdrawalFee for simplicity.

function withdraw(
    uint256 assets,
    address receiver,
    address owner
) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {
    if (receiver == address(0)) revert InvalidReceiver();

    shares = convertToShares(assets);

    uint256 withdrawalFee = uint256(fees.withdrawal);

    uint256 feeShares = shares.mulDiv(
        withdrawalFee,
        1e18 - withdrawalFee,
        Math.Rounding.Down
    );

    shares += feeShares;

    if (msg.sender != owner)
        _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

    _burn(owner, shares);

    if (feeShares > 0) _mint(feeRecipient, feeShares);

    adapter.withdraw(assets, receiver, address(this));

    emit Withdraw(msg.sender, receiver, owner, assets, shares);
}

Shares are calculated through convertToShares, rounding down

function convertToShares(uint256 assets) public view returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return
        supply == 0
            ? assets
            : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
}

With these specific conditions, the share calculation will round to zero.

assets * _totalSupply / _totalAssets => 999 * 100_000 / 100_000_000

Total shares are 0.

Finally, Alice receives her assets, but she burns 0 shares.

_burn(owner, shares);

if (feeShares > 0) _mint(feeRecipient, feeShares);

adapter.withdraw(assets, receiver, address(this));

Vault burns at least one share through adapter.withdraw because it will round up.

Alice has stolen 999 assets, didn't lose any shares, and she can repeat until all the Vault shares in the Adapter are zero.

Tools Used

Manual review

Recommended Mitigation Steps

Round up the share calculation when users withdraw (like the adapter logic), or revert the transaction when a withdrawal would result in 0 shares burned.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L529-L542 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460

Vulnerability details

This issue applies to both AdapterBase.sol and Vault.sol. For the sake of simplicity and brevity, this POC will describe just the former.

Impact

Fee calculation is wrong and it either takes too few or too many shares than what is supposed to be when calculating the accruedPerformanceFee and the shares decimals are not 18. The former causes a loss of shares that the FEE_RECIPIENT should earn, but the latter causes hyperinflation, which makes users' shares worthless.

Proof of Concept

accruedPerformanceFee doesn't take into consideration the shares' decimals, and it supposes that it's always 1e18.

This is supposed to be a percentage and it's calculated as the following, rounding down.

function accruedPerformanceFee() public view returns (uint256) {
    uint256 highWaterMark_ = highWaterMark;
    uint256 shareValue = convertToAssets(1e18);
    uint256 performanceFee_ = performanceFee;

    return
        performanceFee_ > 0 && shareValue > highWaterMark_
            ? performanceFee_.mulDiv(
                (shareValue - highWaterMark_) * totalSupply(),
                1e36,
                Math.Rounding.Down
            )
            : 0;
}

This calculation is wrong because the assumption is:

totalSupply (1e18) * performanceFee_ (1e18) = 1e36

which is not always true because the totalSupply decimals can be greater or less than that.

Let's see what would happen in this case.

Best case scenario: supply decimals < 1e18

In this case, the fee calculation will always round to zero, thus the FEE_RECIPIENT will never get the deserved accrued fees.

Worst case scenario: supply decimals > 1e18

The FEE_RECIPIENT will get a highly disproportionate number of shares.

This will lead to share hyperinflation, which will also impact the users, making their shares worthless.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the fee calculation so it's divided with the correct denominator, that takes into account the share decimals:

performanceFee_ > 0 && shareValue > highWaterMark_
? performanceFee_.mulDiv(
    (shareValue - highWaterMark_) * totalSupply(),
    1e18 * (10 ** decimals()),
    Math.Rounding.Down
)
: 0;
``

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L392 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L110-L122 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L147-L165

Vulnerability details

Impact

Users could receive 0 shares and thus lose their entire investment when making a deposit.

Proof of Concept

Alice calls deposit with 999 assets, with herself as the receiver

function deposit(uint256 assets, address receiver)
    public
    virtual
    override
    returns (uint256)
{
    if (assets > maxDeposit(receiver)) revert MaxError(assets);

    uint256 shares = _previewDeposit(assets);
    _deposit(_msgSender(), receiver, assets, shares);

    return shares;
}

Shares are calculated through _previewDeposit, which uses _convertToShares rounding down

function _convertToShares(uint256 assets, Math.Rounding rounding)
    internal
    view
    virtual
    override
    returns (uint256 shares)
{
    uint256 _totalSupply = totalSupply();
    uint256 _totalAssets = totalAssets();
    return
        (assets == 0 || _totalSupply == 0 || _totalAssets == 0)
            ? assets
            : assets.mulDiv(_totalSupply, _totalAssets, rounding);
}

With specific conditions, the share calculation will round to zero. Let's suppose that _totalSupply = 100_000 and _totalAssets = 100_000_000, then:

assets * _totalSupply / _totalAssets => 999 * 100_000 / 100_000_000

which rounds to zero, so total shares are 0.

Finally, the deposit is completed, and the adapter mints 0 shares.

function _deposit(
    address caller,
    address receiver,
    uint256 assets,
    uint256 shares
) internal nonReentrant virtual override {
    IERC20(asset()).safeTransferFrom(caller, address(this), assets);
    
    uint256 underlyingBalance_ = _underlyingBalance();
    _protocolDeposit(assets, shares);
    // Update the underlying balance to prevent inflation attacks
    underlyingBalance += _underlyingBalance() - underlyingBalance_;

    _mint(receiver, shares);

    harvest();

    emit Deposit(caller, receiver, assets, shares);
}

Alice has lost 999 assets and she has received nothing in return.

Tools Used

Manual review

Recommended Mitigation Steps

Revert the transaction when a deposit would result in 0 shares minted.

Summary

Low-Risk Issues

Total issues: 5

Non-Critical Issues

Total issues: 4

Low Risk

[L-01] Variable shadowing

Context:

31 results - 3 files

src/utils/MultiRewardStaking.sol

- owner

128: if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);
130: _burn(owner, shares);
133: emit Withdraw(caller, receiver, owner, assets, shares);
467: owner,
470: nonces[owner]++,
481: if (recoveredAddress == address(0) || recoveredAddress != owner) revert InvalidSigner(recoveredAddress);

src/vault/adapter/abstracts/AdapterBase.sol

- asset
- owner

60: address asset,
72: __ERC4626_init(IERC20Metadata(asset));
178: if (assets > maxWithdraw(owner)) revert MaxError(assets);
182: _withdraw(_msgSender(), receiver, owner, assets, shares);
193: if (shares > maxRedeem(owner)) revert MaxError(shares);
196: _withdraw(_msgSender(), receiver, owner, assets, shares);
217: if (caller != owner) {
218: _spendAllowance(owner, caller, shares);
228: _burn(owner, shares);
234: emit Withdraw(caller, receiver, owner, assets, shares);
656: owner,
659: nonces[owner]++,
670: if (recoveredAddress == address(0) || recoveredAddress != owner)

src/vault/Vault.sol

- owner

72: __Owned_init(owner);
230: if (msg.sender != owner)
231: _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);
233: _burn(owner, shares);
239: emit Withdraw(msg.sender, receiver, owner, assets, shares);
260: if (msg.sender != owner)
261: _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);
271: _burn(owner, shares);
277: emit Withdraw(msg.sender, receiver, owner, assets, shares);
688: owner,
691: nonces[owner]++,
702: if (recoveredAddress == address(0) || recoveredAddress != owner)

Recommendation Add an underscore to the variable (for example _owner) to avoid shadowing the storage variable.

[L-02] ReentrancyGuardUpgradeable is not initialized

Context

src/vault/adapter/abstracts/AdapterBase.sol

70: __Owned_init(_owner);
71: __Pausable_init();
72: __ERC4626_init(IERC20Metadata(asset));      

Description AdapterBase extends ReentrancyGuardUpgradeable, but is missing __ReentrancyGuard_init() in the __AdapterBase_init function.

[L-03] Missing zero address check for nominatedOwner

Context

2 results - 2 files

src/utils/OwnedUpgradeable.sol

19: function nominateNewOwner(address _owner) external virtual onlyOwner {
20:    nominatedOwner = _owner;
21:    emit OwnerNominated(_owner);
22: }

src/utils/Owned.sol

17: function nominateNewOwner(address _owner) external virtual onlyOwner {
18:    nominatedOwner = _owner;
19:    emit OwnerNominated(_owner);
20: }

Description Missing zero address check for new owners: even if it's not possible to transfer the ownership due to 2-step ownership, this could lead to a failed deploy due to misconfiguration.

[L-04] OwnedUpgradeable should inherit Owner instead of duplicating its code

Context

src/utils/OwnedUpgradeable.sol

src/utils/Owned.sol

[L-05] Immutable variables are not immutable

Context

src/vault/DeploymentController.sol

23: ICloneFactory public cloneFactory;
24: ICloneRegistry public cloneRegistry;
25: ITemplateRegistry public templateRegistry;

Description There are some variables under the IMMUTABLES title that are not immutable: they could be overridden by inheriting contracts.

Non-Critical

[NC-01] Use constant for constants instead of immutable

Context

src/vault/VaultController.sol

36: bytes32 public immutable VAULT = "Vault";
37: bytes32 public immutable ADAPTER = "Adapter";
38: bytes32 public immutable STRATEGY = "Strategy";
39: bytes32 public immutable STAKING = "Staking";

[NC-02] Wrong/missing Natspec

Context

The @notice is wrong, as the caller could be anyone:

src/vault/DeploymentController.sol

/**
 * @notice Adds a new category for templates. Caller must be owner. (`VaultController` via `AdminProxy`)
 * @param templateCategory Category of the new template.
 * @param templateId Unique Id of the new template.
 * @param template New template (See ITemplateRegistry for more details)
 * @dev (See TemplateRegistry for more details)
 */
function addTemplate(
  bytes32 templateCategory,
  bytes32 templateId,
  Template calldata template
) external {
  templateRegistry.addTemplate(templateCategory, templateId, template);
}

Most functions miss @return.

Some functions miss @param or @inheritdoc (most are view functions):

src/utils/MultiRewardEscrow.sol
src/utils/MultiRewardStaking.sol
src/vault/adapter/abstracts/AdapterBase.sol
src/vault/adapter/abstracts/WithRewards.sol
src/vault/adapter/beefy/BeefyAdapter.sol
src/vault/adapter/yearn/YearnAdapter.sol
src/vault/AdminProxy.sol
src/vault/PermissionRegistry.sol
src/vault/TemplateRegistry.sol
src/vault/Vault.sol
src/vault/VaultRegistry.sol

[NC-03] Lock pragmas to a specific compiler version

Context

All contracts

Description: Pragma statements are appropriate when the contract is a library that is intended for consumption by other developers. Otherwise, the developer would need to manually update the pragma, in order to compile locally.

As a best practice it's better to lock the pragma to a specific version.

[NC-04] Showing the full-length numbers in comments increase readability

src/vault/adapter/yearn/YearnAdapter.sol

- 25:      uint256 constant DEGRADATION_COEFFICIENT = 10**18;
+ 25:      uint256 constant DEGRADATION_COEFFICIENT = 10**18; // 1_000_000_000_000_000_000