GiuseppeDeLaZara - Pending allowances can be exploited

[sherlock-admin2]

GiuseppeDeLaZara

high

Pending allowances can be exploited

Summary

Pending allowances can be exploited in multiple places in the codebase.

Vulnerability Detail

TOFT::marketRemoveCollateralReceiver has the following flow:

• It calls removeCollateral ona a market with the following parameters: from = msg_user, to = msg_.removeParams.magnetar.
• Inside the SGLCollateral::removeCollateral _allowedBorrow is called and check if the from = msg_user address has given enough allowanceBorrow to the msg.sender which in this case is the TOFT contract.
• So for a user to use this flow in needs to call:

  function approveBorrow(address spender, uint256 amount) external returns (bool) {
      _approveBorrow(msg.sender, spender, amount);
      return true;
  }

• And give the needed allowance to the TOFT contract.
• This results in collateral being removed and transferred into the Magnetar contract with yieldBox.transfer(address(this), to, collateralId, share);.
• The Magnetar gets the collateral, and it can withdraw it to any address specified in the msg_.withdrawParams.

This is problematic as the TOFT::marketRemoveCollateralReceiver doesn't check the msg.sender. In practice this means if Alice has called approveBorrow and gives the needed allowance with the intention of using the marketRemoveCollateralReceiver flow, Bob can use the marketRemoveCollateralReceiver flow and withdraw all the collateral from Alice to his address.

So, any pending allowances from any user can immediately be exploited to steal the collateral.

Other occurrences

There are a few other occurrences of this problematic pattern in the codebase.

TOFT::marketBorrowReceiver expects the user to give an approval to the Magnetar contract. The approval is expected inside the _extractTokens function where pearlmit.transferFromERC20(_from, address(this), address(_token), _amount); is called. Again, the msg.sender is not checked inside the marketBorrowReceiver function, so this flow can be abused by another user to borrow and withdraw the borrowed amount to his address.

TOFT::mintLendXChainSGLXChainLockAndParticipateReceiver also allows to borrow inside the BigBang market and withdraw the borrowed amount to an arbitrary address.

TOF::exerciseOptionsReceiver has the _internalTransferWithAllowance function that simply allows to transfer TOFT tokens from any _options.from address that has given an allowance to srcChainSender, by anyone that calls this function. It allows to forcefully call the exerciseOptionsReceiver on behalf of any other user.

USDO::depositLendAndSendForLockingReceiver also expects the user to give an allowance to the Magnetar contract, i.e. MagnetarAssetXChainModule::depositYBLendSGLLockXchainTOLP calls the _extractTokens.

Impact

The impact of this vulnerability is that any pending allowances from any user can immediately be exploited to steal the collateral/borrowed amount.

Code Snippet

• https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/TapiocaZ/contracts/tOFT/modules/TOFTMarketReceiverModule.sol#L161
• https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/TapiocaZ/contracts/tOFT/modules/TOFTMarketReceiverModule.sol#L108

Tool used

Manual Review

Recommendation

There are multiple instances of issues with dangling allowances in the protocol. Review all the allowance flows and make sure it can't be exploited.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

seem invalid to me as the approval is made withing the function call which means user doesn't have to call the said approve function

[nevillehuang]

request poc

[sherlock-admin3]

PoC requested from @windhustler

Requests remaining: 2

[windhustler]

Let's imagine Alice has some collateral inside the Singularity Market on Avalanche.

She wants to remove that collateral and initiates a transaction from Ethereum.

Her transaction on Avalanche will call TOFTMarketReceiverModule::marketRemoveCollateralReceiver where Market::removeCollateral through IMarket(msg_.removeParams.market).execute(modules, calls, true);is called.

## SGLCollateral.sol

function removeCollateral(address from, address to, uint256 share)
        external
        optionNotPaused(PauseType.RemoveCollateral)
        solvent(from, false)
        allowedBorrow(from, share)
        notSelf(to)
    {
        _removeCollateral(from, to, share);
    }

/**
* @inheritdoc MarketERC20
     */
    function _allowedBorrow(address from, uint256 share) internal virtual override {
        if (from != msg.sender) {
            // TODO review risk of using this
>>>            (uint256 pearlmitAllowed,) = penrose.pearlmit().allowance(from, msg.sender, address(yieldBox), collateralId); // Alice needs to give allowance to TOFT
>>>            require(allowanceBorrow[from][msg.sender] >= share || pearlmitAllowed >= share, "Market: not approved"); // Alice needs to give allowance to TOFT.
            if (allowanceBorrow[from][msg.sender] != type(uint256).max) {
                allowanceBorrow[from][msg.sender] -= share;
            }
        }
    }

    function _removeCollateral(address from, address to, uint256 share) internal {
        userCollateralShare[from] -= share;
        totalCollateralShare -= share;
        emit LogRemoveCollateral(from, to, share);
        yieldBox.transfer(address(this), to, collateralId, share);
    }

• Remove collateral is called with msg.sender = TOFT, from = Alice, to = Magnetar, and share = 10;
• And then Magnetar withdraws the collateral on another chain to Alice's address or any other address that is set in MagnetarWithdrawData.LzSendParams.SendParam.to, i.e. this can be any address.

So prerequisite for this flow to work is that Alice has:

a) Given allowance to the TOFT contract through the Pearlmit contract. b) Given the allowance to the TOFT contract through the allowanceBorrow function.

In other words, Alice needs to call in a separate transaction:

Singularity.approveBorrow(TOFT, 10)

and

PermiC.approve(address(yieldBox), collateralId, address(TOFT), 10, block.timestamp + 1 hour);

But if Alice has ever given the two allowances listed above, Bob can front-run Alice's TOFTMarketReceiverModule::marketRemoveCollateralReceiver transaction and just call it with the following params:

• from = Alice
• MagnetarWithdrawData.LzSendParams.SendParam.to = Bob
• As a consequence, Bob will steal Alice's collateral.

This is possible due to two reasons:

## TOFTMarketReceiverModule.sol
{
            uint256 share = IYieldBox(ybAddress).toShare(assetId, msg_.removeParams.amount, false);
>>>            approve(msg_.removeParams.market, share);

            (Module[] memory modules, bytes[] memory calls) = IMarketHelper(msg_.removeParams.marketHelper)
                .removeCollateral(msg_.user, msg_.withdrawParams.withdraw ? msg_.removeParams.magnetar : msg_.user, share);
            IMarket(msg_.removeParams.market).execute(modules, calls, true);
        }

• This approve is useless here. In the normal cross-chain call the msg.sender is the lzEndpoint so the approve does nothing. As I have described approvals should be given separately.
• marketRemoveCollateralReceiver is coded in a way that msg.sender is irrelevant which ties to the point above.

To give an analogy, this is almost as Alice giving allowance to UniswapV3 to use her tokens and then Bob can just exploit this allowance to drain Alice's funds. It would make sense if Alice has given the allowance to Bob for using her funds, but this is not the case here.

Let me know if this makes sense or if you need further clarification.

[nevillehuang]

@windhustler This seems to be a duplicate of #31

[windhustler]

31 Makes the claim if Alice gives the allowance to Bob, he can abuse it under certain conditions. And it specifies a single instance related to buyCollateral flow.

My issue makes the claim that if Alice gives allowance to TOFT to execute a simple cross-chain flow, i.e.TOFT::marketRemoveCollateralReceiver, Bob can come along and steal all the collateral from Alice. It's quite different as Alice hasn't given any allowance to Bob at all. It makes the impact and mitigation different.

My issue also states several other occurrences that are similar in nature.

[nevillehuang]

@cryptotechmaker What do you think? I think this could be the primary issue and #31 and duplicates could be duplicated. Would the mitigation be different between these issues?

[cryptotechmaker]

@nevillehuang wouldn't this one and #19 be more or less duplicates?

These are the PRs I did for 19, which might solve it as well

https://github.com/Tapioca-DAO/Tapioca-bar/pull/348

https://github.com/Tapioca-DAO/TapiocaZ/pull/172

[cryptotechmaker]

Issue #137 is similar as well with the difference that 137 mentioned about some missing approvals. However it's still related to the allowance system

[nevillehuang]

@cryptotechmaker Here are the issues related to allowances that seems very similar:

19

31

137

140

Finding it hard to decide on duplication, will update again. Are the fixes similar in these issues?

[cryptotechmaker]

@nevillehuang I would add https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/134 on that list as well

[cryptotechmaker]

We'll analyze them this week but yes, I think those are duplicates

[nevillehuang]

@cryptotechmaker I believe

19 to be a duplicate of #134

31 to be a duplicate of #140

137 Separate issue

[HollaDieWaldfee100]

Escalate

The report explains how in a regular cross-chain flow where TOFT::marketRemoveCollateralReceiver gets called it is expected of the user to give the allowance to the TOFT contract. Setting allowances is a precondition for this flow to be possible, not some extra requirement.  Then it describes how this can be abused by an attacker to steal all the user’s tokens. There are other issues that describe how user loss occurs while another cross-chain flow is being used in a “valid use case scenario”: https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/130 Based on the arguments above there is no special precondition here so this should be a valid high.

[sherlock-admin2]

Escalate

The report explains how in a regular cross-chain flow where TOFT::marketRemoveCollateralReceiver gets called it is expected of the user to give the allowance to the TOFT contract. Setting allowances is a precondition for this flow to be possible, not some extra requirement.  Then it describes how this can be abused by an attacker to steal all the user’s tokens. There are other issues that describe how user loss occurs while another cross-chain flow is being used in a “valid use case scenario”: https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/130 Based on the arguments above there is no special precondition here so this should be a valid high.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

Watson has demonstrated very well how a malicious user can front-run an honest user and steal his allowance, and in this way, he can steal his collateral.

So I plan to accept the escalation and make this issue High.

[Evert0x]

Result: High Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• HollaDieWaldfee100: accepted

[0xRektora]

As a reference: #109 fixes this and any related dangling allowance