existing _exitRequests has the possibility to be over-written by new request leading to loss of existing requests and thus loss of fund.

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xbc9cbbec3b76afb68b40ff1ab9f3a895dfa5f24a8437b4a36e9b0be1d234c43d Severity: high

Description: Description\ existing _exitRequests has the possibility to be over-written by new request leading to loss of existing requests and thus loss of fund.

enterExitQueue is the function for user to signal an exit. The request would be store within _exitRequests with the Id calculated as follow

    // calculate position ticket
    positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;
    // add to the exit requests
    _exitRequests[keccak256(abi.encode(receiver, positionTicket))] = shares;

where as the getLatestTotalTickets is the cummulative tickets.

When the user tries to claimExitedAssets, the requestId would be processed.

...
    // clean up current exit request
    delete _exitRequests[queueId];

    // skip creating new position for the shares rounding error
    if (leftShares > 1) {
      // update user's queue position
      newPositionTicket = positionTicket + claimedShares;
      _exitRequests[keccak256(abi.encode(msg.sender, newPositionTicket))] = leftShares;
    }

If there is some leftShare, the request would be put back into the _requestId, using a newPositionTicket, which is the sum of original positionTicket and claimedShare.

The problem is that the _exitRequests as a result of the leftShare, has the possibility to be over-written by a new request.

Consider a following setup:

getLatestTotalTickets() = 100 queuedShare = 10

bob calls enterExitQueue with 10 shares. He now has a requestId of 110

// 100 + 10 = 110
positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;

After some times, he calls claimExitedAssets, since the actual claimed assets is dependent on the queue conditions. Assume he manages to claim 5 shares, with 5 leftShares. he would then get assigned a new positionId 100 + 5 = 105

// skip creating new position for the shares rounding error
    if (leftShares > 1) {
      // update user's queue position
      newPositionTicket = positionTicket + claimedShares;
      _exitRequests[keccak256(abi.encode(msg.sender, newPositionTicket))] = leftShares;
    }

Now, since queuedShare can be updated during updateState based on the lastest unclaimedAsset, assume the queuedShare is updated to be 5 now. Then If bob initialize a new request of 10 shares through enterExitQueue now, his existing requestId would be over-written.

// 100 + 5 = 105
positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;
_exitRequests[keccak256(abi.encode(receiver, positionTicket))] = shares;

Attack Scenario\ There is also a possibility that a malicious user would monitor/manipulate the queuedShares, and try to overwrite it to any targeted user who has an existing requestId as a result of leftShare. In case of finding one the malicious user could just do a donation attack of 1 wei of shares to overwrite the victium, who then lose claim to the underlying vault and thus loss of fund.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Consider adding a check on enterExitQueue such that it reverts when there is an existing ticket for that positionId

   function enterExitQueue(
   uint256 shares,
   address receiver
   ) public virtual override returns (uint256 positionTicket) {
   _checkCollateralized();
   if (shares == 0) revert Errors.InvalidShares();
   if (receiver == address(0)) revert Errors.ZeroAddress();
   
   // SLOAD to memory
   uint256 _queuedShares = queuedShares;
   
   // calculate position ticket
   positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;
   +++ require(_exitRequests[keccak256(abi.encode(receiver, positionTicket))] == 0, "existing _existRequestId");
   // add to the exit requests
   _exitRequests[keccak256(abi.encode(receiver, positionTicket))] = shares;

[tsudmi]

"Then If bob initialize a new request of 10 shares through enterExitQueue now, his existing requestId would be over-written." - the new position ticket will be 110 as _exitQueue.getLatestTotalTickets() will be updated to 105 after updateState call.