`tOLP` positions created through `MagnetarAction.Permit` can be stolen

[c4-bot-6]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/Magnetar.sol#L199-L212 https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/Magnetar.sol#L304-L305

Vulnerability details

Description

This issue is a combination of three other issues:

• MagnetarAction.TapToken integration will leave tokens stuck in Magnetar contract
• a lot of calls in MagnetarAction.Permit enables anyone to steal whitelisted tokens held by Magnetar (L-01 in QA report)
• anyone can take any whitelisted tokens approved to Magnetar

In short, the first issue describes that, due to how MagnetarAction.TapToken is setup, it will leave the tOLP position a user creates through TapiocaOptionBroker::participate stuck in the Magnetar contract. As it mints the position to msg.sender which will be the Magnetar contract:

File: tap-token/contracts/options/TapiocaOptionBroker.sol

301:        oTAPTokenID = oTAP.mint(msg.sender, lockExpiry, uint128(target), _tOLPTokenID);

The two following ones, describes how anyone can take any tokens that are in the Magnetar contract.

Either by granting themselves permissions to transfer any whitelisted tokens in using MagnetarAction.Permit. Because of how MagnetarStorage::_checkSender validates that the first argument in the calldata to MagnetarAction.Permit is the same as msg.sender. This together with that a lot of the calls allowed through MagnetarAction.Permit (IYieldBox::setApprovalForAll, IYieldBox::setApprovalForAsset, IERC20::approve, and IERC721::approve) have address to i.e. the operator/approvee as the first argument. Hence any user can approve themselves to transfer tokens out of the contract.

Or by using MagnetarAction.OFT. Which allows anyone to transfer tokens out of Magnetar (and steal any approved tokens to Magnetar) since there is an unvalidated call done to any whitelisted contract in MagnetarAction.OFT.

The contract is not in itself supposed to hold any tokens hence in itself these issues are not that severe by themselves.

However these issues combined allows an attacker to steal the position completely. Since the first one makes the position be stuck in Magnetar and the two last ones makes it not actually stuck, but retrievable by anyone.

Impact

If a user uses MagnetarAction.TapToken to create their position they can have their position and/or rewards stolen.

Proof of Concept

Test in tap-token/test/Magnetar.t.sol, builds on the test described in MagnetarAction.TapToken integration will leave tokens stuck in Magnetar contract:

    address attacker = makeAddr("Attacker");

    function testStealTokensStuckInMagnetar() public {
        testMagnetarParticipateOTAPStuckInMagnetar();

        MagnetarCall memory approve = MagnetarCall({
            id: MagnetarAction.Permit,
            target: address(oTAP),
            value: 0,
            allowFailure: false,
            call: abi.encodeWithSelector(IERC721.setApprovalForAll.selector, address(attacker), true)
        });

        MagnetarCall[] memory calls = new MagnetarCall[](1);
        calls[0] = approve;

        vm.startPrank(attacker);
        magnetar.burst(calls);
        oTAP.transferFrom(address(magnetar), address(attacker), 1);
        vm.stopPrank();

        assertEq(oTAP.ownerOf(1),address(attacker));
    }

Full test with setup can be found here.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider implementing the mitigations described in the three mentioned referenced issues:

• Adding the ability for the caller to declare a receiver in TapiocaOptionBroker::participate and exerciseOption. Similar to how it's done in TapiocaOptionLiquidityProvision::lock.
• Rethinking how MagnetarAction.Permit should work. As there is support in modules for a lot of the calls it is used for perhaps it is unnecessary to have.
• Removing the general MagnetarAction.OFT call. Most of the interactions with the contracts in the Tapioca ecosystem is already handled in the Magnetar module system which handles approvals and transfers.

Assessed type

Invalid Validation

[c4-sponsor]

cryptotechmaker marked the issue as disagree with severity

[cryptotechmaker]

The last 2 seems possible only if you approve the attacker, which is valid even for any random ERC20

The first 1 can be an issue, but I don't think the severity should be H. Maybe Medium or Low. I'll let @0xRektora to confirm as well

[0xRektora]

While this is a valid finding, the probability of it happening are very low. Will still put it as medium due to the nature of the issue.

MagnetarAction.TapToken integration will leave tokens stuck in Magnetar contract a lot of calls in MagnetarAction.Permit enables anyone to steal whitelisted tokens held by Magnetar (L-01 in QA report)

We don't actually use TapToken singular actions, instead we use the MagnetarOptionModule. As for permits, we use TapiocaOmnichainEngine/OFT for that.

The probability are very low because the flow of action taken to the casual user is dictated by a different code. While this is possible to happen it'd have to be an advanced user, who probably will batch the actions together of permitting/locking/sending the token to his address. The Tx will revert if those are not done in the same Tx using Magnetar batching

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

0xRektora (sponsor) confirmed

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as selected for report