sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

Minato7namikazi - critical missed check in the `expire()` function: #681

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

Minato7namikazi

High

critical missed check in the expire() function:

Vulnerability Detail

in the expire() function:

function expire() external onlyAdmin {
    require(expiryTime != 0,"expiry not started");
    require(expiryTime < block.timestamp,"no expiry time");

    IERC20(underlyingToken).burn(IERC20(underlyingToken).balanceOf(address(this)));
}

The bug is that this function burns all the underlying tokens held by the contract without checking if there are still outstanding option tokens. This could lead to a situation where users still hold option tokens, but the underlying tokens have been burned, making it impossible for them to exercise their options.

To solve this , we should add a check to ensure that all option tokens have been burned or exercised before allowing the underlying tokens to be burned. Here's a suggested fix:

function expire() external onlyAdmin {
    require(expiryTime != 0, "expiry not started");
    require(expiryTime < block.timestamp, "no expiry time");
    require(totalSupply == 0, "outstanding options exist");

    IERC20(underlyingToken).burn(IERC20(underlyingToken).balanceOf(address(this)));
}

By adding the require(totalSupply == 0, "outstanding options exist"); check, you ensure that all option tokens have been burned or exercised before allowing the underlying tokens to be burned. This protects option token holders from losing their ability to exercise their options due to premature burning of the underlying tokens.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/OptionTokenV4.sol#L542

Tool used

Manual Review

nevillehuang commented 3 months ago

Invalid, this is an admin only functionality where in there is a dedicated expiryTime before the admin can invoke this function, allowing users ample time to exercise options.

ii. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.