Popcorn contest - KingNFT's results

Lines of code

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

Vulnerability details

Impact

The claimRewards() function of MultiRewardStaking contract is susceptible to reentrancy attack. When the reward token is an ERC777 token, attackers can exploit it to drain all the reward tokens in the MultiRewardStaking contract.

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

Proof of Concept

As shown in L170~L188 of MultiRewardStaking contract

  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]); // @audit L174

      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); // @audit L182 reentrancy  point
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

      accruedRewards[user][_rewardTokens[i]] = 0; // @audit L186
    }
  }

the claimRewards() function (1) Have no reentrancy protection such as nonReentrant modifier of openzeppelin-upgradeable lib (2) Violated the CEI rule, which clear the accrued reward (L186) after transfer token to user (L182)

So when the reward token is an ERC777 token, attackers can reentrancy call claimRewards() function at L182, as the accrued reward is cleared at L186 (accruedRewards[user][_rewardTokens[i]] = 0) after reentrancy point, the security check at L174 (if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i])) in reentrancy call will be passed.

By repeating the reentrancy call, all the reward tokens can be drained out.

Tools Used

VS Code

Recommended Mitigation Steps

Add nonReentrant protection or move L186 to L177

Lines of code

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

Vulnerability details

Impact

_withdraw() function of MultiRewardStaking contract calls accrueRewards modifier with incorrect parameters, Users would loss their rewards when caller is not the owner.

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

Proof of Concept

The accrueRewards modifier is designed to update global and user's reward before any balance change.

  modifier accrueRewards(address _caller, address _receiver) {
    IERC20[] memory _rewardTokens = rewardTokens;
    for (uint8 i; i < _rewardTokens.length; i++) {
      IERC20 rewardToken = _rewardTokens[i];
      RewardInfo memory rewards = rewardInfos[rewardToken];

      if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
      _accrueUser(_receiver, rewardToken);

      if (_receiver != _caller) _accrueUser(_caller, rewardToken);
    }
    _;
  }

The _withdraw() function wrongly calls accrueRewards modifier with incorrect parameters

  function _withdraw(
    address caller,
    address receiver,
    address owner,
    uint256 assets,
    uint256 shares
-  ) internal override accrueRewards(caller, receiver) { // @audit should be owner
+  ) internal override accrueRewards(owner, receiver) { // @audit fix
    if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

    _burn(owner, shares);
    IERC20(asset()).safeTransfer(receiver, assets);

    emit Withdraw(caller, receiver, owner, assets, shares);
  }

If a withdraw operation gets called for another user, the owner's pending reward is missing to be accrued and can't be recovered.

Tools Used

VS Code

Recommended Mitigation Steps

Call accrueRewards modifier with correct parameters.

Lines of code

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

Vulnerability details

Impact

The calculation of takeFees in Vault contract is incorrect, which will cause fee charged less than expected.

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

Proof of Concept

To simplify the problem, let's given the fee parameters are as follows

// Fees are set in 1e18 for 100% (1 BPS = 1e14)
struct VaultFees {
    uint64 deposit; // 0
    uint64 withdrawal; // 0
    uint64 management; // 0.5e18 = 50%
    uint64 performance; // 0
}

And the initial asset token and share tokens in the vault are

totalAsset = 100 $AST
totalShare = 100 $SHR
yieldEarnings = 0 $AST

The yield earnings is also set to 0. As the yearly management fee is 50%, so the expected fee for one year is

feeInAsset = 100 $AST * 0.5 = 50 $AST

Now let's calculate the actual fee. The implementation of accruedManagementFee() is

    function accruedManagementFee() public view returns (uint256) {
        uint256 managementFee = fees.management;
        return
            managementFee > 0
                ? managementFee.mulDiv(
                    totalAssets() * (block.timestamp - feesUpdatedAt),
                    SECONDS_PER_YEAR,
                    Math.Rounding.Down
                ) / 1e18
                : 0;
    }

So in this case, one year later, the calculation of first step for managementFee will be

managementFee = 0.5e18 * 100 $AST  * SECONDS_PER_YEAR / SECONDS_PER_YEAR  / 1e18 = 50 $AST

The implementation of takeFees() is

    modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee)); // @audit L491

        _;
    }

So, variables before L491 of takeFees() will be

managementFee = 50 $AST
totalFee  = 50 $AST + 0 = 50 $AST
currentAssets  = 100 $AST

As the implementation of convertToShares() is

    function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply();

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

So the the second parameter for _mint() call at L491 is

feeInShare = convertToShares(totalFee) = convertToShares(50 $AST) = 50 $AST * 100 $SHR / 100 $AST = 50 $SHR

After _mint() at L491, the variables will be

shareOfUser = 100 $SHR
shareOfFeeRecipient = 50 $SHR
totalShare = 100 + 50  = 150 $SHR
totalAsset = 100 $AST

The implementation of convertToAssets() is

    function convertToAssets(uint256 shares) public view returns (uint256) {
        uint256 supply = totalSupply(); 

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

Now we can get actual fee by calling convertToAsset(), which is

actualFeeInAsset = convertToAsset(feeInShare) = convertToAsset(50 $SHR) = 50 $SHR * 100 $AST / 150 $SHR = 33.3 $AST

We can see the actual fee is less than expected, the realistic parameters will be less than the give 0.5e18, but it will be always true that the actual fee charged is not enough.

Tools Used

VS Code

Recommended Mitigation Steps

Use the correct formula such as

    modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

-       if (totalFee > 0 && currentAssets > 0)
-           _mint(feeRecipient, convertToShares(totalFee));
+       if (totalFee > 0 && currentAssets > 0) {
+           uint256 supply = totalSupply();
+           uint256 feeInShare = supply == 0
+               ? totalFee
+               : totalFee.mulDiv(supply, currentAssets - totalFee, Math.Rounding.Down);
+           _mint(feeRecipient, feeInShare);
+       }

        _;
    }