ctf_sec - Refund timestamp expiration is incorrectly extended

[github-actions]

ctf_sec

medium

Refund timestamp expiration is incorrectly extended

Summary

Refund deadline is incorrectly extended

Vulnerability Detail

In the current implementation,

When the user fund the bounty contract, he can set the expiration time:

function fundBountyToken(
	address _bountyAddress,
	address _tokenAddress,
	uint256 _volume,
	uint256 _expiration,
	string memory funderUuid
) external payable onlyProxy {

according to the natspec, the expiration is:

/// @param _expiration The duration until the deposit becomes refundable

As shown in the test case:

console.log('Funding contracts for Client 1...');
	await depositManager.fundBountyToken(openQBounty1Address, mockLink.address, one, thirtySeconds, funderUuid);

the parameter thirtySeconds means that the funded amout of token expires in thirty seconds.

The funder can also have the also to extend the deposit fund expiration time:

/// @notice Extends the expiration for a deposit
/// @param _bountyAddress Bounty address
/// @param _depositId The deposit to extend
/// @param _seconds The duration to add until the deposit becomes refundable
function extendDeposit(
	address _bountyAddress,
	bytes32 _depositId,
	uint256 _seconds
) external onlyProxy {

which calls:

uint256 newExpiration = bounty.extendDeposit(
	_depositId,
	_seconds,
	msg.sender
);

which calls:

/// @notice Extends deposit duration
/// @param _depositId The deposit to extend
/// @param _seconds Number of seconds to extend deposit
/// @param _funder The initial funder of the deposit
function extendDeposit(
	bytes32 _depositId,
	uint256 _seconds,
	address _funder
) external virtual onlyDepositManager nonReentrant returns (uint256) {
	require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);
	require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
	require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);

	if (
		block.timestamp > depositTime[_depositId] + expiration[_depositId]
	) {
		expiration[_depositId] = block.timestamp - depositTime[_depositId] + _seconds;
	} else {
		expiration[_depositId] = expiration[_depositId] + _seconds;
	}

	return expiration[_depositId];
}

note the core logic:

if (
	block.timestamp > depositTime[_depositId] + expiration[_depositId]
) {
	expiration[_depositId] = block.timestamp - depositTime[_depositId] + _seconds;
} else {
	expiration[_depositId] = expiration[_depositId] + _seconds;
}

let us go through an example:

the depositTime is at timestamp 100
the expiration is 10, meaning after 10 seconds, the funder can claim the deposit

now the funder calls extendDeposit and wants to extend the expiration by 5 seconds.

at timestamp 105, 105 is smaller than 100 + 10, therefore the code below executes:

expiration[_depositId] = expiration[_depositId] + _seconds;

expiration time is extended by 5 seconds, which 10 seconds + 5 seconds = 15 seconds.

at timestamp 150, 150 is larger than 100 + 10, therefore the code below executes:

expiration[_depositId] = block.timestamp - depositTime[_depositId] + _seconds;

expirationTime time is set to 150 - 100 + 5 seconds = 45 seconds.

the users means to extend the deposit to 5 seconds but extends the expirationTime to 45 times, which is much longer than user expected.

Impact

The user can unexpectedly extend the deposit refund deadline to a time longer than he expected.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L75-L106

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

Tool used

Manual Review

Recommendation

We recommend the protocol revisit the logic below to make sure the expiration extended is not longer than user expected.

expiration[_depositId] = block.timestamp - depositTime[_depositId] + _seconds;

Duplicate of #552