Lack of Share-to-Amount Conversion in Undelegation Process Can Lead to Token Discrepancies for Users

[hats-bug-reporter[bot]]

Github username: @4gontuk Twitter username: 4gontuk Submission hash (on-chain): 0xb325be032a5ec3b0a2b0a7aae34d675ac01214f2bb251068aeaf0d32b39b5a39 Severity: medium

Description:

Description:

The stROSEMinter contract handles the delegation and undelegation of tokens using shares. The undelegate and takeReceiptUndelegate functions are responsible for initiating and finalizing the undelegation process, respectively. However, these functions fail to properly account for the conversion between shares and token amounts, which can lead to significant discrepancies in the token amounts received by users.

Relevant Components:

1. stROSEMinter Contract:

   • Manages the delegation and undelegation of tokens.
   • Uses the Subcall library to interact with the Oasis Runtime SDK.

2. Subcall Library:

   • Provides functions to perform subcalls to the Oasis Runtime SDK.
   • Handles the actual delegation and undelegation processes.

Functions in Play:

1. stROSEMinter::undelegate Function:

   • Initiates the undelegation process by specifying the number of shares to undelegate.
   • Records the undelegation in the undelegationReceipts mapping.

   function undelegate(StakingAddress from, uint128 shares) public onlyOwner
   {
      Delegation storage d = delegations[from];
      require(shares > 0, "ZeroUndelegate");
      require(d.shares >= shares, "NotEnoughShares");
      uint64 receiptId = nextReceiptId++;
      Subcall.consensusUndelegate(from, shares, receiptId);
      undelegationReceipts[receiptId] = UndelegationReceipt({
          exists: true,
          from: from,
          blockNumber: block.number,
          receiptTaken: false,
          receiptTakenBlockNumber: 0,
          shares: shares,
          epoch: 0,
          endReceiptId: 0
      });
      emit Undelegate(from, shares, receiptId);
   }

2. stROSEMinter::takeReceiptUndelegate Function:

   • Processes the undelegation receipt to finalize the undelegation.
   • Updates the delegation shares and emits an event.

   function takeReceiptUndelegate(uint64 receiptId) public onlyOwner {
      UndelegationReceipt storage receipt = undelegationReceipts[receiptId];
      require(block.number > receipt.blockNumber, "ReceiptNotReady");
      require(receipt.exists == true, "ReceiptNotExists");
      require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
      (uint64 epoch, uint64 endReceiptId) = Subcall.consensusTakeReceiptUndelegateStart(receiptId);
      Delegation storage d = delegations[receipt.from];
   
      // update receipt with the necessary info
      receipt.receiptTaken = true;
      receipt.receiptTakenBlockNumber = block.number;
      receipt.epoch = epoch;
      receipt.endReceiptId = endReceiptId;
   
      // update delegation amount and shares
      d.shares -= receipt.shares;
   
      if (d.shares == 0) {
          _removeDelegation(receipt.from);
      }
   
      emit TakeReceiptUndelegate(receiptId, epoch, endReceiptId);
   }

Root Cause:

The root cause of the issue is the failure to convert shares back to the actual token amount during the undelegation process. This can lead to discrepancies as the value of shares can change over time due to rewards or slashing.

Highest Impact Scenario:

1. User delegates 100 ROSE and receives 100 shares.
2. Due to rewards, these 100 shares now represent 110 ROSE.
3. User undelegates 50 shares.
4. The contract will only record the undelegation of 50 shares, not accounting for the fact that these shares now represent 55 ROSE.

Impact

This bug can lead to significant discrepancies between the expected and actual amounts of tokens undelegated. Users might receive more or fewer tokens than they should, potentially leading to loss of funds or unfair distribution of rewards. This can undermine user trust and the overall token economics of the protocol.

Attack Scenario:

1. Owner calls stROSEMinter::delegate to delegate 100 ROSE and receives 100 shares.
2. Due to rewards, the value of shares increases, and 100 shares now represent 110 ROSE.
3. Owner calls stROSEMinter::undelegate to undelegate 50 shares.
4. The contract records the undelegation of 50 shares without converting them to the actual token amount.
5. Owner calls stROSEMinter::takeReceiptUndelegate to finalize the undelegation.
6. The contract updates the delegation shares but does not account for the actual token amount, leading to discrepancies.

Proof of Concept

1. Delegation:

   • Alice delegates 100 ROSE and receives 100 shares.
   • Due to rewards, these 100 shares now represent 110 ROSE.

   function delegate(StakingAddress to, uint128 amount) public onlyOwner returns (uint64) {
      require(amount < type(uint128).max, ">MaxUint128");
      require(amount > 0, "ZeroDelegate");
      uint64 receiptId = nextReceiptId++;
      Subcall.consensusDelegate(to, amount, receiptId);
      delegationReceipts[receiptId] = DelegationReceipt({
          exists: true,
          to: to,
          blockNumber: block.number,
          receiptTaken: false,
          receiptTakenBlockNumber: 0,
          shares: 0,
          amount: amount
      });
      emit Delegate(to, amount, receiptId);
      return receiptId;
   }

2. Undelegation:

   • Alice undelegates 50 shares.
   • The contract records the undelegation of 50 shares without converting them to the actual token amount.

   function undelegate(StakingAddress from, uint128 shares) public onlyOwner
   {
      Delegation storage d = delegations[from];
      require(shares > 0, "ZeroUndelegate");
      require(d.shares >= shares, "NotEnoughShares");
      uint64 receiptId = nextReceiptId++;
      Subcall.consensusUndelegate(from, shares, receiptId);
      undelegationReceipts[receiptId] = UndelegationReceipt({
          exists: true,
          from: from,
          blockNumber: block.number,
          receiptTaken: false,
          receiptTakenBlockNumber: 0,
          shares: shares,
          epoch: 0,
          endReceiptId: 0
      });
      emit Undelegate(from, shares, receiptId);
   }

3. Finalizing Undelegation:

   • Alice calls takeReceiptUndelegate to finalize the undelegation.
   • The contract updates the delegation shares but does not account for the actual token amount, leading to discrepancies.

   function takeReceiptUndelegate(uint64 receiptId) public onlyOwner {
      UndelegationReceipt storage receipt = undelegationReceipts[receiptId];
      require(block.number > receipt.blockNumber, "ReceiptNotReady");
      require(receipt.exists == true, "ReceiptNotExists");
      require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
      (uint64 epoch, uint64 endReceiptId) = Subcall.consensusTakeReceiptUndelegateStart(receiptId);
      Delegation storage d = delegations[receipt.from];
   
      // update receipt with the necessary info
      receipt.receiptTaken = true;
      receipt.receiptTakenBlockNumber = block.number;
      receipt.epoch = epoch;
      receipt.endReceiptId = endReceiptId;
   
      // update delegation amount and shares
      d.shares -= receipt.shares;
   
      if (d.shares == 0) {
          _removeDelegation(receipt.from);
      }
   
      emit TakeReceiptUndelegate(receiptId, epoch, endReceiptId);
   }

[ilzheev]

Accounting of tokens per each user is not needed, because it's accounted via stROSE.

A user mints 100 stROSE and receives staking rewards via wstROSE ERC4626 vault.

When user wants to exit, he unwraps wstROSE into stROSE and requests withdrawal into ROSE.

If needed, multisig admin undelegates tokens to meet withdrawal request from user – the calculation of the exact amount is not needed, multisig admin can unstake slightly more to make sure user will be able to withdraw in full.