`exitPosition` allows to exit to self and steal funds due to lack of msg.sender / ownership checks

[c4-bot-1]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/Magnetar/Magnetar.sol#L293-L314

Vulnerability details

Impact

Magnetar allows calling TapiocaOptionBroker.exitPosition, but there's no check to verify that the position belongs to the caller

This allows any attacker to exit positions that are approved to Magnetar and Sweep the underlying tokens to themselves

https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/Magnetar/Magnetar.sol#L293-L314

    function _processTapTokenOperation(
        address _target,
        bytes calldata _actionCalldata,
        uint256 _actionValue,
        bool _allowFailure
    ) private {
        if (!cluster.isWhitelisted(0, _target)) revert Magnetar_NotAuthorized(_target, _target);

        bytes4 funcSig = bytes4(_actionCalldata[:4]);
        if (
            funcSig == ITapiocaOptionBroker.exerciseOption.selector
                || funcSig == ITapiocaOptionBroker.participate.selector
                || funcSig == ITapiocaOptionBroker.exitPosition.selector /// @audit Can exit to self and steal funds
                || funcSig == ITapiocaOptionLiquidityProvision.lock.selector
                || funcSig == ITapiocaOptionLiquidityProvision.unlock.selector
        ) {
            _executeCall(_target, _actionCalldata, _actionValue, _allowFailure);
            return;
        }
        revert Magnetar_ActionNotValid(MagnetarAction.TapToken, _actionCalldata);
    }

Any position that was allowed to be managed by Magnetar can be exited by magnetar

Meaning that an attacker could simply be the first to exit and then sweep all funds off of Magnetar

Leaving the original depositors with nothing

This is due to a lack of check for ownership on the TOB Position, which combined with the claimPermission changes, allows us to steal the tokens as any random caller

POC

• Call exitPosition on any lock that has given approval to Magnetar
• Sweep the position to self

Mitigation

twTAP and Magnetar have had multiple integration bugs that are mostly tied to the ability of Magnetar (a singleton) to claim on behalf of multiple actors

In my opinion this is a fundamental issue with this architeture as it tends to cause issues when simply checking for permissions

If you wish to maintain a similar set of permission checks, you would be forced to deploy one Magnetar per user, as a sort of a macro

Alternatively, you'll have to change the claim permissions to be more granular and not front-runnable (permit will not work)

Assessed type

Invalid Validation

[c4-judge]

dmvt marked the issue as duplicate of #170

[c4-judge]

dmvt marked the issue as duplicate of #100

[c4-judge]

dmvt marked the issue as satisfactory