Incorrect implementation of `is_active()` function

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xa7698dc8d025c1d716ebcdd81caf726832a39caed1ed64fec42be2e6fa5e9836 Severity: low

Description: Description\

is_active() is used to check whether the farm is active or not.

        fn is_active(&self) -> bool {
            let current_timestamp = self.env().block_timestamp();
            current_timestamp >= self.start && current_timestamp < self.end
        }

However, there is an issue with current implementation which is shown as above, Here, self.start and self.end are the timestamps of farms being start and end. To understand issue consider a scenario with farm start timestamp is equal to current block.timestamp and end time of farm is 1 days,

current_timestamp = 1705651400;

self.start = 1705651400;

self.end = 1,70,56,55,000; // 1 days duration of farm end

While checking the status of is_active, the first condition current_timestamp >= self.start will always pass and i.e (True), however the other condition current_timestamp < self.end can fail in certain cases. since it checks < self.end so the function in our scenario won't check i.e self.start = 1,70,56,55,000; as 1,70,56,55,000 wont be acheived to compare exactly. so the function will work incorrectly due to some loss of seconds while cheking the status of farm.

This issue will arise due to few seconds difference but the use of is_active is super important in farm contract and it is used in functions like owner_start_new_farm(), owner_withdraw_token().

Recommendedation\

The use of is_active should be precise while using in said contract functions.

        fn is_active(&self) -> bool {
            let current_timestamp = self.env().block_timestamp();
-            current_timestamp >= self.start && current_timestamp < self.end
+           current_timestamp >= self.start && current_timestamp <= self.end
        }

[deuszx]

You wrote:

the first condition current_timestamp >= self.start will always pass and i.e (True)

this is not necessarily true since the farm can be set to start in the future (i.e. when calling owner_start_farm the start can be set to the future).

The farm duration here is used to properly calculate the rewards paid out and currently it distributes rewards from farm.start (inclusive) until farm.end (exclusive).

The resolution here is in seconds, while the block production is not precise-to-a-second so the potential differences will arise but their negligible and unavoidable.

Please note that challange description requires the submission of PoC file:

Proof of Concept (PoC) file: You must provide a file containing a proof of concept (PoC) that demonstrates the vulnerability you have discovered.

[0xRizwan]

Hi @deuszx,

While the rule is correct and POC is submitted for Medium/High severity issue and i don't think the issue is very complex to require POC understanding. I submitted as low severity and issue is straightforward and correct mitigation is provided to resolve the issue.

while checking the active status of farm, the code should check current_timestamp <= self.end instead of current_timestamp < self.end. current_timestamp <= self.end will consider exact timestamp when the farm is supposed to end.

Let me know if you need any further clarification.

[DamianStraszak]

Please provide a POC demonstrating the vulnerability. PoC is required for every level.

[0xRizwan]

@deuszx

Isn't this issue valid?? I have described the scenario in report and i think the issue is understandable theorotically.

[deuszx]

You didn't provide a PoC demonstrating the vulnerability. Your first comment wasn't fully correct. I don't understand why it's a vulnerability to have < instead of <= . Are there some funds left in the farm after it's finished? Do user get fewer rewards? It's not clear from your submission or follow-up comments. That's why we asked for the PoC a week ago, almost immediately after your submissions. Since the challenge is completed - we will not accept new PoCs.

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

It does not show vulnerability in the contract. The suggestion is a design choice, not a fix for a bug.

We hope you participate in the future audits of ink!.