Popcorn contest - eccentricexit's results

Lines of code

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

Vulnerability details

Impact

The claimRewards function of MultiRewardStaking.sol updates the user's accrued rewards after sending the tokens, violating Check-Effects-Interaction. There is also no access or reentry control, which means an adversary can reenter the function and drain the contract.

This is critical if ERC777 token is ever used with this contract.

Proof of Concept

Here is a simplified version of the interaction for better readability.

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {

      // amount to withdraw is read
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];


      // Dangerous token transfer happens here. Before the call is over, the erc777 calls the recipient 
      // which in turns calls `claimRewards` again and the adversary gets to withdraw more tokens.
      token.transfer(msg.sender, rewardsAmount);

     
     // Too late, the state is updated. 
      accruedRewards[user][_rewardTokens[i]] = 0;
    }
  }

Example of an attack contract:

contract Exploit is ERC777TokensRecipient {
    function attack() external {
        stakingContract.claimRewards(...);
    }

    function tokensReceived(...) override external {
        // Here the attacker would add logic to stop reentering
        // before the transaction runs out of gas or the victim
        // contract runs out of tokens.
        // ...

        // Reenter.
        stakingContract.claimRewards(...);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Follow the Check-Effects-Interaction and update accruedRewards before transferring tokens or add guards against reentry.

Lines of code

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

Vulnerability details

Impact

Vault.sol includes a ragequit period so users have a chance to leave to Vault before a change is executed. The code makes it look like it gives the users between 1 and 7 days, but in reality the rage quit duration is always 1 day, because the owner can just decrease it at any moment. The owner can use this to his advantage by setting it to 7 days and after 1 day has passed, drop it to 1 day, locking all users who would've rage quit between 1 and 7 days.

Proof of Concept

There is no proposeQuitPeriod function and nothing stops the owner from calling this whenever they want:

    /**
     * @notice Set a quitPeriod for rage quitting after new adapter or fees are proposed. Caller must be Owner.
     * @param _quitPeriod Time to rage quit after proposal.
     */
    function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
            revert InvalidQuitPeriod();

        quitPeriod = _quitPeriod;

        emit QuitPeriodSet(quitPeriod);
    }

Changes to the fee and the adapter have at most 1 day of rage quit time.

Tools Used

Manual review.

Recommended Mitigation Steps

Implement the propose + accept ragequit pattern for changes to the quite period duration as well:

...
    uint256 public quitPeriod = 3 days;
+   uint256 public proposedQuitPeriod;
+   uint256 public proposedQuitPeriodTime;

+   event QuitPeriodProposed(uint quitPeriod);
    event QuitPeriodSet(uint256 quitPeriod);
...

+ /**
+     * @notice Propose a new quit period duration for rage quitting. Caller must be Owner.
+     * @param _quitPeriod The new quit period proposed.
+     */
+    function proposeQuitPeriod(uint256 _quitPeriod) external onlyOwner {
+        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
+            revert InvalidQuitPeriod();
+
+        proposedQuitPeriod = _quitPeriod;
+        proposedQuitPeriodTime = block.timestamp;
+
+        emit QuitPeriodProposed(_quitPeriod);
+    }

...
-   /**
-     * @notice Set a quitPeriod for rage quitting after new adapter or fees are proposed. Caller must be Owner.
-     * @param _quitPeriod Time to rage quit after proposal.
-     */
-    function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
-        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
-            revert InvalidQuitPeriod();
-
-        quitPeriod = _quitPeriod;
-
-        emit QuitPeriodSet(quitPeriod);
-    }   
+   /**
+     * @notice Accept a new ragequit period duration. Caller must be Owner.
+     */
+    function setQuitPeriod() external onlyOwner {
+        if (block.timestamp < proposedQuitPeriodTime + quitPeriod) revert NotPassedQuitPeriod(quitPeriod);
+
+        quitPeriod = proposedQuitPeriod;
+
+        emit QuitPeriodSet(quitPeriod);
+    }
...

Lines of code

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

Vulnerability details

Impact

Delegate calls to the strategy are supposed to be regulated by a cooldown. However, lastHarverst is never actually updated with block.timestamp which means anyone can call it as much as they want.

    function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {
            // solhint-disable
            address(strategy).delegatecall(
                abi.encodeWithSignature("harvest()")
            );

           // Missing lastHarvest = block.timestamp; here.
        }

        emit Harvested();
    }

We don't know how future strategies will behave. It's better to plug this hole.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Add the missing update to lastHarvest.

Lines of code

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

Vulnerability details

Impact

Vault deposits and withdrawals are supposed to update the high water mark, but users can avoid the sync when burning shares by using the redeem function instead of withdraw as it is lacking the modifier.

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {
    // Withdraw has the modifier.

    ...
}


function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {
      // missing syncFeeCheckpoint modifier.
      ...
 }

Proof of Concept

Tools Used

Manual review.

Recommended Mitigation Steps

Add the missing modifier:

function redeem(
        uint256 shares,
        address receiver,
        address owner
-    ) public nonReentrant  returns (uint256 assets) {
+    ) public nonReentrant syncFeeCheckpoint returns (uint256 assets) {
      ...
}

1 Low Severity

1.1 Inclusion as start time can lead to unexpected behavior.

MultiRewardEscrow uses the transaction inclusion time (block.timestamp) as the starting point of the escrow. This can lead to unintended behavior in times of high transaction fee volatility. In case of sudden gas price increase, an adversary could also hold on to the transaction and re-publish it again when gas prices drop.

Suggestion: Let the user set the starting time of the escrow. It may also be interesting to add a mechanism to prevent republishing of the transaction, such as a nonce or inclusion deadline:

function lock(
    IERC20 token,
    address account,
    uint256 amount,
    uint32 duration,
-   uint32 offset
+   uint32 offset,
+   uint256 startTime
  ) external {
    ...

-    uint32 start = block.timestamp.safeCastTo32() + offset;
+    uint32 start = startTime + offset;

    ...
  }

1.2 Mass reward claim can be DoSed

MultiRewardEscrow.claimRewards and MultiRewardStaking.claimRewards revert if there is a single item with 0 claimable amount. This can make building convenience tools that claim for users hard to build and waste gas if the call is included after one of the items is individually claimed.

Suggestion: continue to the next item instead of reverting so other they can continue being claimed:

  function claimRewards(bytes32[] memory escrowIds) external {
    for (uint256 i = 0; i < escrowIds.length; i++) {
			...

      uint256 claimable = _getClaimableAmount(escrow);
-      if (claimable == 0) revert NotClaimable(escrowId);
+      if (claimable == 0) continue;
      ...
    }
  }

  function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      ...
-      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);
+      if (rewardAmount == 0) continue;
      ...
    }
  }

1.3 isClaimable returns true even for unclaimable escrows

If an escrow is in the offset period, it is not claimable. MultiRewardEscrow.isClaimable, however, will still return true, which is unexpected and can break potential future integrations:

Suggestion: Return false if the escrow is still in the offset period and document the function.

  function isClaimable(bytes32 escrowId) external view returns (bool) {
+   if (block.timestamp < escrows[escrowId].start) return false;
    return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;
  }

1.4 CloneRegistry allows duplicate entries

CloneRegistry.addClone does not check if the item was already added before, pushing a duplicate into the clones mapping and allClones array as well as emitting a new CloneAdded event which can confuse offchain tooling.

Suggestion: Disallow adding a clone that already exists:

  ...
+  error CloneAlreadyExists();
  ...
  function addClone(
    bytes32 templateCategory,
    bytes32 templateId,
    address clone
  ) external onlyOwner {
+   if(cloneExists[clone]) revert CloneAlreadyExists();
    cloneExists[clone] = true;
    clones[templateCategory][templateId].push(clone);
    allClones.push(clone);

    emit CloneAdded(clone);
  }

1.5 Anyone can add a template

According to the documentation, DeploymentController.addTemplate should only be callable by the owner (VaultController or AdminProxy) but it is callable by anyone because it is missing a onlyOwner modifier.

Suggestion: Add the missing onlyOwner modifier:

  /**
   * @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 {
+  ) external onlyOwner {
    templateRegistry.addTemplate(templateCategory, templateId, template);
  }

1.6 Harvest cooldown can have illegal value

Documentation and code on setHarvestCooldown disallow >= 1 day durations, but the adapter can be initialized with any value.

Suggestion: Disallow initializing the adapter with illegal values:

    function __AdapterBase_init(bytes memory popERC4626InitData)
        internal
        onlyInitializing
    {
        ...
+       if (_harvestCooldown >= 1 days) revert InvalidHarvestCooldown(_harvestCooldown);
        harvestCooldown = _harvestCooldown;

        if (_strategy != address(0)) _verifyAndSetupStrategy(_requiredSigs);

				...
    }

2 Code QA

2.1 IMultiRewardEscrow.Fee.accrued field is never used. 2.2 EIP165: Use interface or abstract instead of contract. 2.3 MultiRewardStaking, BeefyAdapter: Instead of declaring _name and _symbol again, use the fields already available from the parent ERC20 contracts by calling __ERC20_init inside initialize. 2.4 WithRewards: Add the missing abstract in the contract declaration. 2.5 AdapterBase.harvest: Only emit Harvested from inside the if since it does not actually harvest if the condition passes. This will also save gas.

3 Documentation QA

3.1 ITemplateRegistry.Template: Remove trailing dots from natspec. 3.2 AdminProxy, VaultController: Typo in documentation. Change Ownes to Owns. 3.3 Vault.changeAdapter: Neither highWaterMark nor assetsCheckpoint are updated here. Remove the misleading docs. 3.4 VaultController.deployVault: Caller does not need to be the owner if address(1) is set to false. Clarify this in the documentation. 3.5 VaultController.deployAdapter: As in 3.4, caller does not need to be the owner if address(1) is set to false. Clarify this in the documentation.