Popcorn contest - fs0c's results

Lines of code

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

Vulnerability details

Impact

In MultiRewardStaking the function claimRewards doesn’t have nonReentrant which makes it possible to re-enter the function. If one of the reward tokens in ERC-777 token, it is possible to re-enter and claim the reward again and again until the contract is drained out of those tokens.

When the function would be re-entered, it would call accrueRewards again which would call _accrueUser for _receiver and _caller, it would update the value of accruedRewards again as accruedRewards[_user][_rewardToken] += supplierDelta; The second time supplierDelta would be zero, but the accruedRewards would remain the same. This value is used later in the claimRewards function as rewardAmount and sent to the user.

POC

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

      accruedRewards[user][_rewardTokens[i]] = 0;
    }
  }

In the above function the accruedRewards[user][_rewardTokens[i]] is updated after the tokens are transfered to the user. This makes it possible to steal the rewards multiple times in case of ERC-777 (extension of ERC-20) tokens, which have a callback.

Recommendation

Add nonReentrant modifier.

Lines of code

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

Vulnerability details

Impact

As the strategy implementation is currently unknown and a user can define it on their own, it is possible for strategy to have some storage variables, and for the harvest function to work upon those storage variables.

The function harvest() in AdapterBase.sol calls harvest in strategy using a delegate call. If in some case, the delegate call modifies storage variables. Those variables would be modified in adapter and might change some important variables in the adapter contract.

Currently we can’t tell the complete effect this has on adapter contract as the strategy contract can have different implementations, but this design can surely lead to some storage collision.

POC

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

        emit Harvested();
    }

Recommendation

If the delegatecall is just used to forward the msg.sender (as written in the comments) , use a different approach rather than a delegatecall.

Lines of code

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

Vulnerability details

Impact

Malicious users can drain the assets of the vault.

POC

The withdraw function users convertToShares to convert the assets to the amount of shares. These shares are burned from the users account and the assets are returned to the user.

The function withdraw is shown below:

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

        shares = convertToShares(assets);
/// .... [skipped the code]

The function convertToShares is shown below:

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

It uses Math.Rounding.Down , but it should use Math.Rounding.Up

Assume that the vault with the following state:

• Total Asset = 1000 WETH
• Total Supply = 10 shares

Assume that Alice wants to withdraw 99 WETH from the vault. Thus, she calls the Vault.withdraw(99 WETH) function.

The calculation would go like this:

assets = 99
return value = assets * supply / totalAssets()
return value = 99 * 10 / 1000
return value = 0

The value would be rounded round to zero. This will be the amount of shares burned from users account, which is zero.

Hence user can drain the assets from the vault without burning their shares.

Note : A similar issue also exists in mint functionality where Math.Rounding.Down is used and Math.Rounding.Up should be used.

Recommendation

Use Math.Rounding.Up instead of Math.Rounding.Down.

As per OZ implementation here is the rounding method that should be used:

deposit : convertToShares → Math.Rounding.Down

mint : converttoAssets → Math.Rounding.Up

withdraw : convertToShares → Math.Rounding.Up

redeem : convertToAssets → Math.Rounding.Down

[QA-01] changeAdapter should only be called by owner.

Impact

Altough there is no immediate threat due to this, but this function should only be called by owner.

POC

function changeAdapter() external takeFees {
        if (block.timestamp < proposedAdapterTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        adapter.redeem(
            adapter.balanceOf(address(this)),
            address(this),
            address(this)
        );

        asset.approve(address(adapter), 0);

        emit ChangedAdapter(adapter, proposedAdapter);

        adapter = proposedAdapter;

        asset.approve(address(adapter), type(uint256).max);

        adapter.deposit(asset.balanceOf(address(this)), address(this));
    }

Recommendation

Add onlyOwner modfier to the function.

[QA-02] Remove the unnecessary variable assetsCheckpoint and change the natspec comments regarding that.

The unnecessary variable assetsCheckpoint can be removed and the natspec comment of changeAdapter has to be modified to remove the variable.

* @notice Set a new Adapter for this Vault after the quit period has passed.
     * @dev This migration function will remove all assets from the old Vault and move them into the new vault
     * @dev Additionally it will zero old allowances and set new ones
     * @dev Last we update HWM and assetsCheckpoint for fees to make sure they adjust to the new adapter
     */
    function changeAdapter() external takeFees {

[QA-03] Function state mutability can be restricted to view

src/utils/MultiRewardStaking.sol:351:3:

function _calcRewardsEnd(
    uint32 rewardsEndTimestamp,
    uint160 rewardsPerSecond,
    uint256 amount
  ) internal returns (uint32) {

src/vault/VaultController.sol:242:3:

function _encodeAdapterData(DeploymentArgs memory adapterData, bytes memory baseAdapterData)
    internal
    returns (bytes memory)

src/vault/VaultController.sol:667:3:

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