[M15] Magnetar's `mintBBLendSGLLockTOLP` reverts when `lock` is set to false

[c4-bot-3]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarMintCommonModule.sol#L68-L93

Vulnerability details

Impact

The mintBBLendSGLLockTOLP function in Magnetar is designed to mint USDO tokens from bigbang, deposit them to singularity, lock the liquidity in the TOLP contract and then participate in the TOB contract.

The function is designed to be modular, so the user can choose to skip any of the steps and still have the other execute. The issue is that if the user decides not lock in the TOLP contract, the function will revert since it is pulling tokens from the wrong address.

After the market operations, the function does two operations for locking, as shown below.

uint256 tOLPTokenId = _lockOnTOB(
    data.lockData,
    yieldBox_,
    fraction,
    data.participateData.participate,
    data.user,
    data.externalContracts.singularity
);

if (data.participateData.participate) {
    _participateOnTOLP(data.participateData, data.user, data.lockData.target, tOLPTokenId);
}

In the _lockOnTOB function, there is a check which allows the user to skip this step based on their input.

if (lockData.lock) {
    //...
}

However, if this step is skipped, then the TOLP NFT position will still be with the user, and thus needs to be pulled from the user for the participate step. But in the participate step, we see that the code expects the token to be with the MAgnetar contract already.

IERC721(lockDataTarget).approve(participateData.target, tOLPTokenId);
uint256 oTAPTokenId = ITapiocaOptionBroker(participateData.target).participate(tOLPTokenId);

address oTapAddress = ITapiocaOptionBroker(participateData.target).oTAP();
IERC721(oTapAddress).safeTransferFrom(address(this), user, oTAPTokenId, "0x");

This would only be true if the previous step's lock function had run. This is because when locking the tokens, the Magnetar contract receives the NFT position if participate function is set to be run.

SO if the lock function is skipped, the participate function will revert since the NFT position is still with the user.

Proof of Concept

During the lock function call, we see that the Magnetar contract tries to credit itself the NFT tokens.

tOLPTokenId = ITapiocaOptionLiquidityProvision(lockData.target).lock(
    participate ? address(this) : user, singularityAddress, lockData.lockDuration, lockData.amount
);

However, if the lock functionality is skipped, then the Magnetar contract will not own the tokens since this step will not be run. Users will be forced to raw send the NFT position to the magnetar contract which is unsafe since the Magnetar contract is not designed to hold tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

If lockData.lock was true, the _participateOnTOLP function should pull the NFT positions from the user to the Magnetsar contract.

Assessed type

Other

[c4-sponsor]

cryptotechmaker (sponsor) confirmed

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt marked the issue as selected for report

[cryptotechmaker]

https://github.com/Tapioca-DAO/tapioca-periph/pull/212/commits/160d198bcdc435f2bccc046016ce7db1bc09575d