Popcorn contest - Breeje's results

Lines of code

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

Vulnerability details

Description

The first deposit with a totalSupply of zero shares will mint shares equal to the deposited amount.

File: src/vault/Vault.sol

298:     supply == 0
299:            ? assets
300:            : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);

Link to Code

File: src/vault/Vault.sol

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

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

        shares = convertToShares(assets) - feeShares;

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

        _mint(receiver, shares);

        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));

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

Link to Code

For calculation Simplicity let's take fees to be zero as of now.

Problems with the code:

1. Integer division negatively affect the user.
2. Can be manipulated to cause a large loss, specifically for first victim.

Impact

It can lead to some part of Fund getting stolen from First Depositor.

Proof of Concept

Consider the following situation:

1. Attacker deposits 1 wei of WETH.
2. Next, Attacker transfers 100 WETH to the contract.
3. Victim deposits 200 WETH.
4. Attacker withdraws 1 share.

Have a look at this table to understand the complete PoC:

Tools Used

Manual Review

Recommended Mitigation Steps

1. Need to Enforce a minimum deposit that can't be withdrawn.
2. So, mint some of the initial amount to the zero address.
3. Most legit first depositors will mint thousands of shares, so not a big cost.

QA Report

[L-1] USE OF FLOATING PRAGMA

Impact: swcregistry

Instances (35):

All File in Scope uses pragma solidity ^0.8.15; Floating pragma Solidity Version.

[L-2] RETURN VALUE OF LOW-LEVEL CALLS NOT CHECKED

Instance (1):

File: src/vault/adapter/abstracts/AdapterBase.sol

444:     address(strategy).delegatecall(
445:          abi.encodeWithSignature("harvest()")
446:     );

Link to Code

[L-3] MISSING EXISTANCE CHECK IN toggleTemplateEndorsement METHOD

In toggleTemplateEndorsement method of TemplateRegistry contract, there is only one check !templateExists[templateId] which checks if the templateId exists or not.

But it is not necessary that the given templateId is mapped to templateCategory which was passed through function parameter. So it is important to check this by adding:

    if(templates[templateCategory][templateId].implementation == address(0)) revert CategoryIdMismatch(templateCategory, templateId);

[L-4] super KEYWORD NOT USED TO CALL PARENT FUNCTIONS

It is recommended to use the super keyword to call a function in parent contract.

Instances (4):

File: src/utils/MultiRewardStaking.sol

76:     return deposit(_amount, msg.sender);

80:     return mint(_amount, msg.sender);

84:     return withdraw(_amount, msg.sender, msg.sender);

88:     return redeem(_amount, msg.sender, msg.sender);

Link to Code

[NC-1] FUNCTION STATE MUTABILITY CAN BE RESTRICTED TO VIEW

Following function instances do not change state of the contract. So it is recommended to restrict its mutability to view only.

Instances (4):

File: src/utils/MultiRewardStaking.sol

351:     function _calcRewardsEnd(

Link to Code

File: src/vault/VaultController.sol

242:     function _encodeAdapterData(DeploymentArgs memory adapterData, bytes memory baseAdapterData)

667:     function _verifyCreatorOrOwner(address vault) internal returns (VaultMetadata memory metadata) {

Link to Code

File: src/vault/strategy/StrategyBase.sol

12:    function verifyAdapterSelectorCompatibility(bytes4[8] memory sigs) public {

Link to Code

[NC-2] AVOID VARIABLE NAMES THAT CAN SHADE

_totalAssets shades with _totalAssets function. Other shades are self explanatory.

Instance (10):

File: src/vault/adapter/abstracts/AdapterBase.sol

388:     uint256 _totalAssets = totalAssets();

Link to Code

File: src/utils/MultiRewardEscrow.sol

117:      token: token,
118:      start: start,
123:      account: account

Link to Code

File: src/utils/MultiRewardStaking.sol

267:     escrowPercentage: escrowPercentage,
268:     escrowDuration: escrowDuration,
269:     offset: offset

280:     ONE: ONE,
281:     rewardsPerSecond: rewardsPerSecond,
282:     rewardsEndTimestamp: rewardsEndTimestamp,

Link to Code

[NC-3] UNUSED LOCAL VARIABLE

Local variable should be removed if not used. Either use this variable in place of state variable to save gas or remove this variable.

Instance (1)

File: src/vault/Vault.sol

448:     uint256 highWaterMark_ = highWaterMark;

Link to Code

[NC-4] UNUSED FUNCTION PARAMETER

Unused parameter can be removed.

Instance (1):

File: src/vault/VaultController.sol

390:     function _registerVault(address vault, VaultMetadata memory metadata) internal {

Link to Code

[NC-5] ERROR DECLARED BUT NOT USED

Error should be removed as it is not used.

Instance (1):

File: src/vault/CloneFactory.sol

32:     error NotEndorsed(bytes32 templateKey);

Link to Code

[NC-6] MISSING CHECKS FOR address(0) OUTSIDE OF AUTOMATED FINDINGS

Instance (1):

File: src/vault/CloneRegistry.sol

44:       address clone

Link to Code

[NC-7] INTERFACE CODE DEVELOPED BUT NOT IMPLEMENTED

The contracts listed below implements all the methods of the interface IPermissionRegistry, IVaultRegistry, IMultiRewardEscrow and IMultiRewardStaking respectively. But those interfaces are not added in is.

File: src/vault/PermissionRegistry.sol

14:       contract PermissionRegistry is Owned {

Link to Code

File: src/vault/VaultRegistry.sol

15:       contract VaultRegistry is Owned {

Link to Code

File: src/utils/MultiRewardEscrow.sol

21:       contract MultiRewardEscrow is Owned {

Link to Code

File: src/utils/MultiRewardStaking.sol

26:       contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {

Link to Code