c4-bot-1 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

The _processWrapOperation function can be triggered on the Magnetar contract to wrap/unwrap the user's tokens into or out of the TOFT contracts. This function allows 2 selectors, wrap and unwrap to be called.

The issue is that unwrap function has 2 issues which prevent it from working properly.

Below is the code from the BaseTOFT.sol contract, showing the implementation of the unwrap function.

function _unwrap(address _toAddress, uint256 _amount) internal virtual {
    _burn(msg.sender, _amount);
    vault.withdraw(_toAddress, _amount);
}

As seen here, the token is burnt from the msg.sender address. However, the _processWrapOperation function in the Magnetar contract does not transfer out the token from the user's address to itself before calling _unwrap.

if (funcSig == ITOFT.wrap.selector || funcSig == ITOFT.unwrap.selector) {
    /// @dev Owner param check. See Warning above.
    _checkSender(abi.decode(_actionCalldata[4:36], (address)));
    _executeCall(_target, _actionCalldata, _actionValue, _allowFailure);
    return;
}

So the Magnetar contract isnt in posession of the token that the TOFT contract is trying to burn. The Magnetar contract itself is not designed to hold user tokens, since anyone can claim them, so user's should not send their tokens to the Magnetar contract manually and then call this function, since MEV bots can steal tokens from this contract by just calling the unwrap function before them. Due to this, there is no way for Magnetar to actually unwrap the tokens.

Secondly, the _checkSender function is used to check the first passed address against msg.sender. the issue is that the first address passed to the unwrap function is the destination address, not the owner's address. The owner is assumed to be msg.sender. So this contract essentially only makes sure that the msg.sender matches the destination of the unwrapped tokens, which is not a useful check.

Since these 2 issues break the functionality of this function, this is a medium severity issue.

Proof of Concept

The fact that Magnetar isnt designed to hold tokens is evident from the fact that any user can just call the unwrap function from magnetar and burn up any tokens it is currently holding. So the absence of the transferring of the token to the Magnetar contract itself causes an issue.

Tools Used

Manual Review

Recommended Mitigation Steps

The unwrap functionality should transfer the token from the msg.sender to itself first. This will ensure both that the caller owns the token and that the token is in the possession of the Magnetar contract. The _checkSender check for the unwrap case is unnecessary and prevents users from choosing a destination address.