HonorLt - Incorrect new expiration when extending expired deposit

[github-actions]

HonorLt

medium

Incorrect new expiration when extending expired deposit

Summary

extendDeposit assigns the wrong new expiration time when the deposit has expired.

Vulnerability Detail

extendDeposit calculates the new expiration differently depending if the deposit has expired or not:

    function extendDeposit(
        bytes32 _depositId,
        uint256 _seconds,
        address _funder
    ) external virtual onlyDepositManager nonReentrant returns (uint256) {
        ...
        if (
            block.timestamp > depositTime[_depositId] + expiration[_depositId]
        ) {
            expiration[_depositId] =
                block.timestamp -
                depositTime[_depositId] +
                _seconds;
        } else {
            expiration[_depositId] = expiration[_depositId] + _seconds;
        }

        return expiration[_depositId];
    }

As you can see, when it has expired, the new expiration time is calculated this way:
block.timestamp - depositTime[_depositId] + _seconds;
This basically translates to the time elapsed since the deposit plus new seconds.

I believe this calculation is wrong. It operates on the elapsed time with no anchor to any specific timestamp. To illustrate why I think this is wrong, let's see this example:

1. I deposited now with an expiration of 5 seconds. The initial deposit expiration is now + 5s, where now is a UNIX timestamp. 1676471088 at the time of writing, so my expiration is 1676471093.
2. After 10 seconds (UNIX timestamp 1676471098), I decided to extend my deposit by 30 seconds. The current algorithm will calculate this new expiration timestamp:
   1676471998 - 1676471088 + 30 = 940
   Clearly, this is a wrong timestamp. It has already passed.

Impact

Now it is practically impossible to extend an expired deposit.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L108-L114

Tool used

Manual Review

Recommendation

I think a simple solution would be to continue from the current timestamp when the deposit is expired:
expiration[_depositId] = block.timestamp + _seconds.
Another more precise option would be allowing users to specify not the seconds but the end timestamp.

[IAm0x52]

Invalid. Watson misunderstands how expiration works. It is the amount of time AFTER the deposit timestamp that the deposit expires NOT the timestamp that it expires. Current logic is working as intended.

[hrishibhat]

Agree with Lead watson comment