anyone can take any whitelisted tokens approved to `Magnetar`

[c4-bot-10]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/Magnetar.sol#L325-L333

Vulnerability details

Description

Magnetar has a call where you can interact with OFT contracts, Magnetar::_processOFTOperation:

File: tapioca-periph/contracts/Magnetar/Magnetar.sol

325:    function _processOFTOperation(
326:        address _target,
327:        bytes calldata _actionCalldata,
328:        uint256 _actionValue,
329:        bool _allowFailure
320:    ) private {
321:        if (!cluster.isWhitelisted(0, _target)) revert Magnetar_NotAuthorized(_target, _target);
322:        _executeCall(_target, _actionCalldata, _actionValue, _allowFailure);
323:    }

_executeCall simply does call on the target contract.

The issue here is that there is no validation other than that the _target is whitelisted.

A lot of functionality in Magnetar requires tokens, specifically Singularity tokens to be whitelisted. They also require the user to approve Magnetar to transfer these tokens as shown in MagnetarBaseModule::_extractTokens. This is called on a singularity in MagnetarMintCommonModule::_lockOnTOB.

Therefore, any user that has an existing approval to Magnetar for their singularity (or any other whitelisted token) can have their approved tokens stolen.

Impact

Any user that has an approval for Magnetar for any token that is whitelisted in Magnetar can have their approved tokens stolen. Since some calls within Magnetar require this, it is likely this will affect users.

This also affects any whitelisted tokens held by the Magnetar contract, but as it is only a helper contract not designed to hold any tokens this isn't as impactful.

Proof of Concept

Test in tap-token/test/MagnetarApproval.t.sol:

    function testStealApprovalERC20() public {
        erc20.mint(address(ada),1e18);

        vm.prank(ada);
        erc20.approve(address(magnetar),1e18);

        MagnetarCall memory transfer = MagnetarCall({
            id: MagnetarAction.OFT,
            target: address(erc20),
            value: 0,
            allowFailure: false,
            call: abi.encodeWithSelector(IERC20.transferFrom.selector, address(ada), address(this), 1e18)
        });

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

        magnetar.burst(calls);

        assertEq(erc20.balanceOf(address(this)),1e18);
    }

The full test setup can be found here.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider 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

Access Control

[cryptotechmaker]

Medium.

The same happens if you approve the attacker for an ERC20.

However, this is worth fixing imo. I think we can add specific selectors instead of allowing any call to be executed.

[c4-sponsor]

cryptotechmaker marked the issue as disagree with severity

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

0xRektora (sponsor) confirmed

[cryptotechmaker]

Fixed by https://github.com/Tapioca-DAO/tapioca-periph/pull/206/commits/d4bb69d70f57d570a9608b797f1effc35cfa8490

[c4-judge]

dmvt marked the issue as duplicate of #100

[c4-judge]

dmvt marked the issue as satisfactory