Lack of Slashing Protection will lead to Incorrect Share Accounting and Potential Financial Discrepancies

[hats-bug-reporter[bot]]

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

Description:

Description:

The stROSEMinter contract is designed to handle delegation and undelegation of tokens to validators on the Oasis consensus layer. It uses a system of shares to represent the amount of tokens staked with validators. The core functionalities include delegating tokens to validators, receiving shares in return, and undelegating shares to retrieve tokens.

Relevant Components:

• Delegation structure: Represents the shares held by a user.
• DelegationReceipt and UndelegationReceipt structures: Track the details of delegation and undelegation transactions.
• delegate(), takeReceiptDelegate(), undelegate(), and takeReceiptUndelegate() functions: Handle the delegation and undelegation processes.

Issue:

The contract assumes a constant relationship between shares and tokens, which is not accurate in proof-of-stake systems where validators can be slashed. Slashing reduces the actual value of shares, but the contract does not reflect this change, leading to incorrect share accounting.

Detailed Examination:

1. Delegation:

   • The delegate() function correctly records the amount delegated and issues a receipt.
   • The takeReceiptDelegate() function updates the shares without considering potential slashing events. It assumes that the shares received are always equivalent to the amount delegated, which is not true if slashing occurs.

   function takeReceiptDelegate(uint64 receiptId) public onlyOwner returns (uint128 shares) {
       DelegationReceipt storage receipt = delegationReceipts[receiptId];
       require(block.number > receipt.blockNumber, "ReceiptNotReady");
       require(receipt.exists, "ReceiptNotExists");
       require(receipt.receiptTaken == false, "ReceiptAlreadyTaken");
       shares = Subcall.consensusTakeReceiptDelegate(receiptId);
       Delegation storage d = delegations[receipt.to];
   
       // update receipt with the necessary info
       receipt.shares = shares;
       receipt.receiptTaken = true;
       receipt.receiptTakenBlockNumber = block.number;
   
       // update delegation amount and shares
       d.shares += shares;
   
       _addDelegation(receipt.to);
       emit TakeReceiptDelegate(receiptId);
   }

2. Undelegation:

   • The undelegate() function allows undelegation based on the number of shares without considering the actual value of those shares post-slashing. This can lead to a situation where the contract tries to withdraw more tokens than it actually has.

   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);
   }

Highest Impact Scenario:

1. User A delegates 1000 ROSE and receives 1000 shares.
2. Validator is slashed, reducing the value of shares by 10%.
3. User A's 1000 shares now represent only 900 ROSE.
4. User B delegates 900 ROSE and receives 1000 shares (due to reduced share value).
5. The contract shows both User A and User B having 1000 shares, but they represent different amounts of ROSE.
6. If both users try to undelegate all their shares, the contract will attempt to withdraw 2000 ROSE worth of shares, but only 1800 ROSE actually exist.

Impact

The contract fails to account for slashing events, leading to incorrect share accounting and potential financial discrepancies. This can result in incorrect representation of user stakes, potential loss of funds for users, contract insolvency if multiple users undelegate after slashing events, and unfair distribution of rewards and losses among users.

Attack Scenario:

1. Owner calls delegate() to delegate 1000 ROSE to a validator.
2. Validator gets slashed, reducing the value of shares by 10%.
3. Owner calls delegate() again to delegate 900 ROSE to the same validator.
4. Owner calls takeReceiptDelegate() to update the shares for both delegations.
5. Owner calls undelegate() to undelegate all shares.
6. The contract attempts to withdraw more tokens than it actually has, leading to financial discrepancies.

Proof of Concept

1. Initial Delegation:

   • User A delegates 1000 ROSE and receives 1000 shares.
   • Code Reference: delegate()

2. Slashing Event:

   • Validator is slashed, reducing the value of shares by 10%.
   • No direct code reference, but this is an external event affecting the share value.

3. Subsequent Delegation:

   • User B delegates 900 ROSE and receives 1000 shares (due to reduced share value).
   • Code Reference: delegate()

4. Receipt Handling:

   • User A and User B call takeReceiptDelegate() to update their shares.
   • Code Reference: takeReceiptDelegate()

5. Undelegation:

   • Both users call undelegate() to undelegate all their shares.
   • Code Reference: undelegate()

6. Financial Discrepancy:

   • The contract attempts to withdraw 2000 ROSE worth of shares, but only 1800 ROSE actually exist.
   • Code Reference: undelegate()

[ilzheev]

Number of shares is not slashed. https://github.com/hats-finance/Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784/issues/18#issuecomment-2326275119