search

hats-finance / Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2

Smart contracts for Kintsu
Other
0 stars 0 forks source link

User who opt for `request_unlock` at the end of epoch could not get sufficient time to apply for `cancel_unlock_request` #42

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: @aktech297 Twitter username: kaka Submission hash (on-chain): 0x2303181153b635d84e7404e7f8043d81bc0eec4455a3e7e6b99c49679a2b520d Severity: medium

Description: Description\ When a user opt for request_unlock at the end of the epcoh will not get sufficient time to raise the cancel_unlock_request as per the implementation.

Attack Scenario\

Attachments

  1. Proof of Concept (PoC) File

When we see the functions request_unlock and cancel_unlock_request, they are stroing and retrivign the requests using the id which is calcualted using the function get_batch_unlock_id

    pub fn get_batch_unlock_id(&self, time: Timestamp) -> u64 {
        (time - self.creation_time) / self.batch_interval_delay
    }

if a user raise the request_unlock, then they would be allowed to cancel it with in this epoch window.

L297-L304

        /// Allow user to cancel their unlock request
        ///
        /// Must be done in the same batch interval in which the request was originally sent
        #[ink(message)]
        pub fn cancel_unlock_request(&mut self, user_unlock_id: u128) -> Result<(), VaultError> {
            let caller = Self::env().caller();
            let now = Self::env().block_timestamp();

But the problem is in the function get_batch_unlock_id which trucate the value. so if a user raise the request at the end of epoch, they will not get the full epoch time to cancel it.

Lets see the following case - simplified one

batch_interval_delay = 2

time starts at 1

creation_time is 1

if we apply this in the function get_batch_unlock_id for different input we will get following output.

For examle, if they raise the ticket at 2.9 , they would be given id with 2. in this case, their cancel request time passes quick.

They will not get enoug window to apply for cancel request.

  1. Revised Code File (Optional)

I think, the restriction in the function cancel_unlock_request can be removed. So that the implementation will be user friendly.

https://github.com/hats-finance/Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2/blob/c9bdc853b18c305de832307b91a9bca0f281f71e/src/vault/lib.rs#L300-L307

        #[ink(message)]
        pub fn cancel_unlock_request(&mut self, user_unlock_id: u128) -> Result<(), VaultError> {
            let caller = Self::env().caller();
            let now = Self::env().block_timestamp(); --- @@ this one.

            let current_batch_unlock_id = self.data.get_batch_unlock_id(now);
            let mut user_unlock_requests = self.data.user_unlock_requests.get(caller).unwrap_or(Vec::new());

And get the input time from user and proceed for cancel.

bgibers commented 1 month ago

This is intended by design. All unlock requests are batched every 48 hours, and sent to the validators. Once the requests have been batched it has already been submitted to the network to withdraw the funds from the validator

i.e. it's not possible to cancel the withdraw while the unbonding process has already began

aktech297 commented 1 month ago

@0xmahdirostami pls share how do you agree as system design.?.

We see users are not able to cancel the unlock request.

Isn't against the user willingness to cancel it.

It would be great if you can apply some meaningfulness to judge the issue here.

bgibers commented 1 month ago

All unlock requests are batched as one and sent to the nomination pool. If there are 100 users requesting to unlock 100 tokens each, over a 48 hour window, a single request to unlock 10000 tokens is sent. Once that request is sent its not possible for a user to cancel their unlock request, as its held in escrow. This is documented within our app as well as in our docs here