anyone can exitPosition for others in TapiocaOptionBroker, coupled with overlapping condition in isPositionActive on exerciseOption, leads to potential frontrun of user

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/options/TapiocaOptionBroker.sol#L296-L307

Vulnerability details

Impact

anyone can exitPosition for others in TapiocaOptionBroker, coupled with overlapping condition in isPositionActive on exerciseOption, leads to potential frontrun of user

First of all, anyone can call exitPosition on any position, given the position expires on the following condition => block.timestamp >= lock.lockTime + lock.lockDuration :

    /// @notice Exit a twAML participation and delete the voting power if existing
    /// @param _oTAPTokenID The tokenId of the oTAP position
    function exitPosition(uint256 _oTAPTokenID) external {
        require(oTAP.exists(_oTAPTokenID), "tOB: oTAP position does not exist");

        // Load data
        (, TapOption memory oTAPPosition) = oTAP.attributes(_oTAPTokenID);
        (, LockPosition memory lock) = tOLP.getLock(oTAPPosition.tOLP);

        require(
            block.timestamp >= lock.lockTime + lock.lockDuration,
            "tOB: Lock not expired"
        );

However, when we look at exerciseOption which relies on tOLP.getLock to check whether an option is exercisable, it is (lockPosition.lockTime + lockPosition.lockDuration) >= block.timestamp;.

    function exerciseOption(
        uint256 _oTAPTokenID,
        ERC20 _paymentToken,
        uint256 _tapAmount
    ) external {
        // Load data
        (, TapOption memory oTAPPosition) = oTAP.attributes(_oTAPTokenID);
        (bool isPositionActive, LockPosition memory tOLPLockPosition) = tOLP
            .getLock(oTAPPosition.tOLP);
...
require(isPositionActive, "tOB: Option expired");
...

In TapiocaOptionLiquidityProvision.sol :

    function getLock(
        uint256 _tokenId
    ) external view returns (bool, LockPosition memory) {
        LockPosition memory lockPosition = lockPositions[_tokenId];

        return (_isPositionActive(_tokenId), lockPosition);
    }

    function _isPositionActive(uint256 tokenId) internal view returns (bool) {
        LockPosition memory lockPosition = lockPositions[tokenId];

        return
            (lockPosition.lockTime + lockPosition.lockDuration) >=
            block.timestamp;
    }

So now we can see when an option is at (lockPosition.lockTime + lockPosition.lockDuration) == block.timestamp; it's both exercisable and exitable, but anyone can exit for the option holder.

This is suboptimal since:

1. it creates conflicting condition where the system is no longer deterministic in its state transition ability at a particular timestamp.

2. Although rare, a user can be front-runned now to prevent him/her from exercising an option.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

either make isPositionActive slightly stricter or expireOption slightly looser, so that they dont overlap.

isPositionActive

        return
---            (lockPosition.lockTime + lockPosition.lockDuration) >=
---            block.timestamp;
+++            (lockPosition.lockTime + lockPosition.lockDuration) >
+++            block.timestamp;
    }

Assessed type

Math

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Consider QA

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as remove high or low quality report

[c4-sponsor]

0xRektora marked the issue as disagree with severity

[0xRektora]

Although not applicable in real world, this can be informational.

[c4-sponsor]

0xRektora marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b

[c4-judge]

dmvt marked the issue as grade-a