`MagnetarAction.Permit` interaction with `Pearlmit` wrongly assumes second argument is owner

[c4-bot-6]

Lines of code

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

Vulnerability details

Description

In Magnetar a user can batch a lot of calls to the Tapioca ecosystem. One of these is the MagnetarAction.Permit. This performs various permit operations. One if the targets of these operations is the IPearlmit.approve operation:

Magnetar::_processPermitOperation:

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

199:        bytes4 funcSig = bytes4(_actionCalldata[:4]);
200:        if (
                    // ...
204:                || funcSig == IERC20.approve.selector || funcSig == IPearlmit.approve.selector
205:                // ...
206:        ) {
207:            /// @dev Owner param check. See Warning above.
208:            _checkSender(abi.decode(_actionCalldata[4:36], (address)));
209:            // No need to send value on permit
210:            _executeCall(_target, _actionCalldata, 0, _allowFailure);
211:            return;
212:        }

Then in MagnetarStorage::_checkSender:

File: tapioca-periph/Magnetar/MagnetarStorage.sol

93:    function _checkSender(address _from) internal view {
94:        if (_from != msg.sender && !cluster.isWhitelisted(0, msg.sender)) {
95:            revert Magnetar_NotAuthorized(msg.sender, _from);
96:        }
97:    }

There is a check that if what is sent as the first argument to the call is != msg.sender then msg.sender must be whitelisted.

The issue is that this code makes an erroneous assumption about IPearlmit::approve:

File: tapioca-periph/contracts/interfaces/periph/IPearlmit.sol

39:    function approve(address token, uint256 id, address operator, uint200 amount, uint48 expiration) external;

Here you see that the first argument is the token contract for which the approval is for. Pearlmit will infer the owner from msg.sender which will be the Magnetar contract.

Hence, for any end user, this can never pass the _from != msg.sender check. For the call to work, every end user must be whitelisted.

Impact

MagnetarAction.Permit interaction with Pearlmit doesn't work as it requires msg.sender to be whitelisted.

Whitelisting end user addresses will enable abuses as described in M-13 in the Pashov audit group report. Since that's not an option, the IPearlmit::approve interaction on Magnetar will not work.

Proof of Concept

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

    function testPearlmitApproval() public {
        MagnetarCall memory pearlmitApprove = MagnetarCall({
            id: MagnetarAction.Permit,
            target: address(pearlmit),
            value: 0,
            allowFailure: false,
            call: abi.encodeWithSelector(
                IPearlmit.approve.selector,
                address(1), // token address, doesn't matter it will not be the same as msg.sender 
                1, address(1), 1, 0
            )
        });

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

        // reverts as the token address != msg.sender
        vm.expectRevert(abi.encodeWithSelector(MagnetarStorage.Magnetar_NotAuthorized.selector, address(this), address(1)));
        magnetar.burst(calls);

        // works when end user is whitelisted
        cluster.updateContract(0, address(this), true);
        magnetar.burst(calls);
    }

Full test setup can be found here

Tools Used

Manual audit

Recommended Mitigation Steps

Consider removing this functionality as the owner is inferred from the caller which will always be the Magnetar contract.

Assessed type

Invalid Validation

[c4-sponsor]

cryptotechmaker (sponsor) confirmed

[c4-judge]

dmvt marked the issue as primary issue

[cryptotechmaker]

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

[c4-judge]

dmvt marked the issue as selected for report

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-a

[c4-judge]

dmvt marked the issue as not selected for report