`LM_PC_Staking_v1` can be used for pyramid scheme, because it is using current stakers funds to reward others

[hats-bug-reporter[bot]]

Github username: @NicolaMirchev Twitter username: EgisSec Submission hash (on-chain): 0x6ecbce60bcd9552924e1d97a4071d452c97d7300d0fe3cf05a4625e452e0af95 Severity: medium

Description: Description\ LM_PC_Staking_v1 is inheriting from ERC20PaymentClientBase_v1, which means it is treated as a regular paymentClient module, but this is not correct approach, because in the case of staking, end users are providing their funds, which are used for incentivising other stakers. (Users play the role of a funding manager). And only when staking contract balance doesn't have the funds for given operation, it would try to pull funds from FundingManager contract. But there is no guarantee that the funding manager would be funded, which would result in an expoit of type pull-rug + pyramid. If a malicious party has noticed that contract functionality is using stakers funds to incetives other stakers, he can create a staking contract with very attractive rewardRate and rewardToken, which would attract users and last ones to try getting their funds back would be deceived. The problem is that every time a user interact with the contract stake/unstake, his recurred rewards are being processed and sent to him in a way that uses other stakers rewards: We start from _distributeRewards

        _addPaymentOrder(
            PaymentOrder({
                recipient: recipient,
                paymentToken: address(orchestrator().fundingManager().token()),
                amount: amount,
                start: block.timestamp,
                cliff: 0,
                end: block.timestamp
            })
        );

        __Module_orchestrator.paymentProcessor().processPayments(
            IERC20PaymentClientBase_v1(address(this))
        );

here we call paymentProcessor::processPayments , than back to staking contract -> ERC20PaymentClientBase_v1::collectPaymentOrders. This function will check if current contract (staking) have enough funds to distribute to recipients. Most of the time, this would be the case, because this is the contract, where user funds are staying.

Attack Scenario\

• An expliter has notices the issue that staking contract is using paymentClient direct logic, which in terms of staking is practically rug-pull weapon
• He creates an orchestrator and module trough the protocol and set very attractive reward rate of 20% APR for ethereum.
• There would be many victims, which would see that, especially if Inverter would have a place to look for projects, which have been deployed using their framework
• The system will work great in the beginning and the expoiter himself may open a large stake position for himself, which would later benefit from his followers
• In some point users will start unstaking, which would be okay, untill all funds are withdrawn from the protocol (some of them for incetivicing other users)
• Last users to unstake would have lost their funds

Attachments

1. Proof of Concept (PoC) File Will provide in comments if requested

2. Revised Code File (Optional) If module owner is responsible for providing funds to his project (payment scheduler, or something like this), it is okay to use this approach with the payment client. But here the code allow end users to enter in the role of funders, which is not secure. To mitigate the issue override _ensureTokenBalance in LM_PC_Staking_v1 and modify it as follows to ensure that ones staked funds are not used to incentives others.

   - uint currentFunds = IERC20(token).balanceOf(address(this));
   + uint currentFunds = IERC20(token).balanceOf(address(this)) - totalSupply;
       // If current funds are not enough
       if (currentFunds < amount) {
       // always withdraw reward funds from funding manager
       ...

[PlamenTSV]

The only way that user balances inside the contract impact the rewards distributed is by changing the total_supply. Since the rewardRate is set inside setRewards when the total supply is still 0, you cannot claim more than the initially funded amount. So if you have 10_000 in rewards and 10_000 staked total supply, users cannot claim more than the 10_000. Another aspect you mentioned is a malicious orchestrator and a bit of civil engineering, which I think are OOS, per the sponsors comment (will wait for his opinion on this matter to be perfectly sure).

I will give you an example of the first point, correct me if I misunderstood the issue or made a mistake in my example: For simplicity lets say we call setRewards: amount = 10_000, duration = 10_000, rewardRate = 1, lastUpdate = 0 for simplicity since its the timestamp of creation, rewardValue becomes 0 on initialization

Now lets say 1000 seconds pass, Bob stakes 1000. rewardValue is still 0 since the total supply is updated at the end, lastUpdate = 1000, earned for bob = 0 Now lets say the period runs out, Bob is the only staker and calls claimRewards(). This time totalSupply is updated, so rewardValue = 10_000 - 1000 = 9900 1 (rewardRate) / 1000 (totalSupply) = 9 (rounding uint) => lastUpdate = 10_000, earned = (9 - 0 (since on the only deposit, the value was 0)) 1000 = 9000

The total supply is 1000 and Bob can withdraw them any time he wants, not only because he is the only staker, but because the rewardRate is proportional to the initially added reward, not the total amount of tokens inside the contract.

As I said, correct me if I am wrong.

[NicolaMirchev]

Hey,

Answering to judge statement

I don't think you understand my point. My concern is that the funds, which are currently in LM_PC_Staking_v1 would be used as rewards to other stakers, before the fundingManager part is hit. I believe this is an issue even if the orchestrator is not malicious and there is a situation, where his staking program has a large TVL, which he hasn't predicted and fundingManager doesn't have the funds to incentivize all stakes. Calculations in your example are correct and my concern is not regarding them.

• Bob won't be able to either claim, or withdraw if there are not enough funds in fundingManager, which automatically is an issue, which doesn't require orchestrator to be malicious. This is the case, because if he try to only withdraw his 1000 tokens, the function call would try to distribute him all 9000 rewards (which currently we don't have in the contract), so we would try to pull them from fudingManager and revert if funding manager also doesn't have them:

  function _ensureTokenBalance(address token) internal virtual {
      uint amount = _outstandingTokenAmounts[token];
      uint currentFunds = IERC20(token).balanceOf(address(this));
  
      // If current funds are not enough
      if (currentFunds < amount) {
          // check if the token is the FudningManager token and transfer it
          if (
              token == address(__Module_orchestrator.fundingManager().token())
          ) {
              // Trigger callback from orchestrator to transfer tokens
              // to address(this).
              bool ok;
              (ok, /*returnData*/ ) = __Module_orchestrator
                  .executeTxFromModule(
                  address(__Module_orchestrator.fundingManager()),
                  abi.encodeCall(
                      IFundingManager_v1.transferOrchestratorToken,
                      (address(this), amount - currentFunds)
                  )
              );
  
              if (!ok) {
                  revert Module__ERC20PaymentClientBase__TokenTransferFailed();
              }
          } else {
              revert Module__ERC20PaymentClientBase__InsufficientFunds(token);
          }
      }
  }

  This is a serious concern because even if orchestrator is not malicious, he cannot have control of the stakers funds flow in the system.

Further developing the issue

Actually when I was trying to explain that to you, now I see there is even further impact, which I could classify as a High, because it is constantly present issue, which result in lost funds when the reward token is set to be the same as the staking token

• Imagine that after Bob, Alice stakes another 9000 tokens
• Now totalSupply would be 10000 and Bob would be able to claim his 9000 rewards and withdraw his 1000 stakes, leaving the staking contract balance empty. Those 9000 would be Alice funds, because contract won't try to pull tokens from funding manager, while the contract still has the balance.

• Now when Alice won't be able to withdraw her stakes, because staking contract will first try to transfer her the stakingAmount (which is missing from the account) and then try to pull only rewards amount from fundingManager:

  function unstake(uint amount)
      external
      virtual
      nonReentrant
      validAmount(amount)
  {
      address sender = _msgSender();
      // Update rewardValue, updatedTimestamp and earned values
      _update(sender); 
  
      // Reduce balances accordingly
      _balances[sender] -= amount;
      // Total supply too
      totalSupply -= amount;
  
      // Transfer funds back to sender
      IERC20(stakingToken).safeTransfer(sender, amount); // REVERT WOULD HAPPEN HERE BECAUSE WE DON"T HAVE BALANCE
  
      // If the user has earned something
      if (rewards[sender] != 0) {
          // distribute rewards
          _distributeRewards(sender); //// HERE REWARD PULLING FROM MANAGER HAPPENS
      }
  
      emit Unstaked(sender, amount);
  }

• I used term "pyramid", because if there are other stakers after Alice, she will take their funds, when unstaking, but this would further mess up the accouting.

Now you can see different impacts, which root is my concern -> Staking contract priorities stakers funds for rewards distribution, before pulling funds from fundingManager

[PlamenTSV]

Staking contract priorities stakers funds for rewards distribution, before pulling funds from fundingManager The staking contract should be preloaded with funds before rewards are set. If that is not the case, I am validating this, so will wait for the sponsor to comment. It's because personally when I see a setReward function which does not intake funds, I reckon they are sent beforehand. The funding manager should handle only the emergency situations, but generally the staking contract should always have X+Y funds, X being the rewards and Y being the user stakes.

[NicolaMirchev]

I believe this is the whole idea behind having a fundingManager as a separate module. Protocol architecture make it responsible for funding all payments, which are being processed by paymetClient modules. One such is the staking module.

The tokens can be sent before or after. The protocol doesn't enforce anything. Considering the protocol is supposed to be modular and is used by a wide variety of users and for different use cases, both cases should be handled correctly

But yeah, let's wait for sponsor assumptions

[NicolaMirchev]

Hey, @PlamenTSV

Could you please add help label here, so it is easier for the sponsor to not miss this one. Thanks!

[PlamenTSV]

Hey, @PlamenTSV

Could you please add help label here, so it is easier for the sponsor to not miss this one. Thanks!

Of course, thanks for pointing it out!

[FHieser]

Yes I would say this is valid. Good find :D Its a very specific usecase and I actually had it in mind while development, but It must have missed it during implementation. Its a quick fix. I think medium is appropriate here. :+1:

[NicolaMirchev]

Hey, @FHieser

Glad to hear that from you! Did you saw my further explanation here. I think this may be more serious, because if conditions are present, impact if high, since there is an avalanche effect on the broken accounting and users funds being locked and lost. And conditions to be present is only to have a reward token same as the staking token. I think we can agree that in this space such staking projects are common (to have reward token = staking token) Having in mind above mentioned points I think an appropriate judgement would be:

Impact = high
Probability = Medium
 -----------
Overall = High