`unlock_id` open for duplication then user can lost their asset when redeeming

[hats-bug-reporter[bot]]

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

Description: Description

User if want to unlock their stake, can call request_unlock. This function will insert pending request for unlock for the user. Each request have their own unlock_id. This unlock_id is generated from

let unlock_id=(user_unlock_requests.len()-1) as u128;, line 282

The issue here is, this unlock_id open for duplication.

Consider this case:

1. Alice staked 100 AZERO
2. She want to unlock 20 AZERO, doing request_unlock, which it will get the unlock_id = 0
3. She then want to unlock another 30 AZERO, which will get unlock_id = 1, and user_unlock_requests length = 2
4. She then cancel_unlock_request of the 20 AZERO, which is unlock_id 0, now user_ulock_requests length is 1
5. When she finally decide she want to take all of her stake, request_unlock 70 AZERO (since there is still request unlock 30 AZERO), then she will get the same unlock_id, which is 1.
6. This will also overwrite the users user_unlock_requests

Finally when Redeem, Alice will use the duplicate unlock_id get only the 70 AZERO, while the 30 AZERO is overwrited

note that, she already transfer her shares sAZERO to contract on line 254

File: lib.rs
250:         pub fn request_unlock(&mut self, shares: Balance) -> Result<(), VaultError> {
251:             let caller = Self::env().caller();
252:             let now = Self::env().block_timestamp();
253: 
254:  @>         self.transfer_shares_from(&caller, &Self::env().account_id(), shares)?;
255: 
256:             let current_batch_unlock_id = self.data.get_batch_unlock_id(now);
257:             let current_batch_unlock_shares = self
258:                 .data
259:                 .batch_unlock_requests
260:                 .get(current_batch_unlock_id)
261:                 .map(|b| b.total_shares)
262:                 .unwrap_or(0);
263: 
264:             // Update current batch unlock request
265:             self.data.batch_unlock_requests.insert(
266:                 current_batch_unlock_id,
267:                 &UnlockRequestBatch {
268:                     total_shares: current_batch_unlock_shares + shares,
269:                     value_at_redemption: None,
270:                     redemption_timestamp: None,
271:                 },
272:             );
273: 
274:             // Update user's unlock requests
275:             let mut user_unlock_requests = self.data.user_unlock_requests.get(caller).unwrap_or(Vec::new());
276:             user_unlock_requests.push(UnlockRequest {
277:                 creation_time: now,
278:                 share_amount: shares,
279:                 batch_id: current_batch_unlock_id,
280:             });
281:             self.data.user_unlock_requests.insert(caller, &user_unlock_requests);
282: @>          let unlock_id=(user_unlock_requests.len()-1) as u128;
283:             Self::emit_event(
284:                 Self::env(),
285:                 Event::UnlockRequested(UnlockRequested {
286:                     staker: caller,
287:                     shares,
288:                     unlock_id,
289:                     batch_id: current_batch_unlock_id,
290: 
291:                 }),
292:             );
293: 
294:             Ok(())
295:         }

The issue arise due to potential duplicate of unlock_id because of user_ulock_requests can be decreased due to cancelation, and then after being overwritted when redeeming using this unlock_id user can lost their asset.

Attack Scenario

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation

Replace the generation of unlock_id implementation. Or maybe when canceling, keep the user_unlock_requests index, just empty the values.

[chainNue]

typo correction, the steps when unlock and cancel should be sAZERO not AZERO

[coreggon11]

When Alice requests unlock of id 0, the id 1 will get id 0. So after new request the new request will get id 1, and the original withdraw request is still there.

You should write a test case that supports this, because it does not seem to be correct.

[chainNue]

Agree. after cancel or redeem, id is adjusted. so its not a fixed id. This submission is invalid.

[0xmahdirostami]

Thanks for your submission, just like issue #2, unlock_id isn’t a saved variable on UnlockRequest it is simply a pointer to the position of the array.