Popcorn contest - y1cunhui's results

Lines of code

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

Vulnerability details

Impact

For rewardTokens that implement callback mechanism like calling onERC20Received function after transfer, the function claimRewards() in MultiRewardStaking.sol can be reentered to drain this rewardToken from the pool.

Proof of Concept

File: src/utils/MultiRewardStaking.sol
170   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 {
              //@audit reentrancy here
            _rewardTokens[i].transfer(user, rewardAmount);
            emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
          }

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

1. Some token T with callback mechanism when transfer is added into rewardTokens
2. The malicious user A is a smart contract that implements corresponding callback function like onERC20Received
3. The malicious user A has gained some rewardAmout > 0
4. User A call function claimReward with corresponding _rewardToken. Assuming the escrowPercentage <= 0 and it goes to the line with comment.
5. After transfer, the malicious callback of user A is called, and in the callback function A call claimRewards again. By this time, the accruedRewards has not been zeroed; and in the accruedRewards(msg.sender, _user) modifier, there is no way the accruedRewards will be decreased. So the user get the same rewardAmount again and again, until gas drained or RewardToken drained.

Tools Used

Manual Review, VS Code

Recommended Mitigation Steps

The vulnerability is because the developer neither used the ReentrancyGuard modifier, nor follow the CEI (check-effects-interactions) pattern, since the accuedRewards zeroed after the transfer call.

As a result, the mitigation steps is just add ReentrancyGuard modifier to this function, or move the line 186 to line 175. Always check functions with external calls, not just ether send/transfer.

[L-01] Consider Add Boundry for `rewardPerSecond" to avoid overflow

When addToken and changeRewardSpeed, there is no boundry for rewardPerSecond. Maybe some basic boundry should be added.

File: src/utils/MultiRewardStaking.sol
function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner

function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner

[L-02] Invalid Permission Control for addTemplate

As the comment of theser two function says: * @notice Adds a new category for templates. Caller must be owner. But in below code snippet, there is no control over caller:

File: src/vault/DeploymentController.sol
  function addTemplate(
    bytes32 templateCategory,
    bytes32 templateId,
    Template calldata template
  ) external {
    templateRegistry.addTemplate(templateCategory, templateId, template);
  }

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

Since the comment here says it should only be called by owner, and in the TemplateRegistry.sol, this function has the modifier onlyOwner where the owner is just the DeploymentController above. Missing modifier here will make the permisson control in TemplateRegistry useless.

[NC-01] Wrong Comment for BPS

When 1e18=100%, 1 BPS should be 1e14 as other comments of this file says.

File: src/vault/ValutController.sol
      /**
       * @notice Sets new fees per vault. Caller must be creator of the vaults.
       * @param vaults Addresses of the vaults to change
       * @param fees New fee structures for these vaults
       * @dev Value is in 1e18, e.g. 100% = 1e18 - 1 BPS = 1e12
       */

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

[NC-02] Confusion caused by repeated use of name owner

Parameter name owner is used here:

File: src/utils/MultiRewardStaking.sol
121   function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
      ) internal override accrueRewards(caller, receiver) {
        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);
      }

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

However, since this contract is inherited from OwnedUpgradeable, where there is also a state variable named owner:

contract OwnedUpgradeable is Initializable {
  address public owner;
  address public nominatedOwner;
    ...

Consider change the parameter name of function _withdraw.

[NC-03] Code length exceeds 80 letters

Just list some of them below for example:

File: src/utils/MultiRewardStaking.sol
    _name = string(abi.encodePacked("Staked ", IERC20Metadata(address(_stakingToken)).name()));

    _symbol = string(abi.encodePacked("pst-", IERC20Metadata(address(_stakingToken)).symbol()));

    function name() public view override(ERC20Upgradeable, IERC20Metadata) returns (string memory) {
    
    function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {