Block-list tokens are un-sellable due to msg.sender hardcoding

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x314a2162070e25d8efeb5fdde4a581788be97df0b376b2e703b8b59c760e615f Severity: low

Description: Description\ The RedeemingBondingCurveBase_v1.sol is a part of the Funding Manager Module and involves locking up collateral for minting issuance tokens. It allows to buy/sell for the user himself and buyFor/sellFor to buy on behalf of someone else. However, block-list ERC20's could make collateral unredeemable for certain users, due to the hardcoded msg.sender address.

Attack Scenario\ This occurs inside _sellOrder and sellFor. _sellOrder takes as parameters a receiver who will be the address whose balance is lowered and collateral transfered to. Due to the function sellFor passing the msg.sender as the receiver, instead of letting the user provide a destination address, the collateral can remain stuck if the msg.sender is blocked by these tokens (USDC, USDT most notably)

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   
   -function sell(uint _depositAmount, uint _minAmountOut) public virtual {
   -        sellFor(_msgSender(), _depositAmount, _minAmountOut);
   -}
   +function sell(uint _depositAmount, uint _minAmountOut, address receiver) public virtual {
   +        sellFor(_msgSender(), receiver, _depositAmount, _minAmountOut);
   +    }

function _sellOrder(

• address holder address _receiver, uint _depositAmount, uint _minAmountOut ) internal returns (uint totalCollateralTokenMovedOut, uint issuanceFeeAmount) { _validateDepositAmount(_depositAmount); // Get protocol fee percentages and treasury addresses ( address collateralTreasury, address issuanceTreasury, uint collateralSellFeePercentage, uint issuanceSellFeePercentage ) = _getFunctionFeesAndTreasuryAddresses( bytes4(keccak256(bytes("_sellOrder(address, uint, uint)"))) );

  uint protocolFeeAmount;
  uint workflowFeeAmount;
  uint netDeposit;
  
  // Get net amount, protocol and workflow fee amounts. Currently there is no issuance project
  // fee enabled
  (netDeposit, protocolFeeAmount, /* workflowFee */ ) =
  _calculateNetAndSplitFees(_depositAmount, issuanceSellFeePercentage, 0);
  
  issuanceFeeAmount = protocolFeeAmount;
  
  // Calculate redeem amount based on upstream formula
  uint collateralRedeemAmount = _redeemTokensFormulaWrapper(netDeposit);
  
  totalCollateralTokenMovedOut = collateralRedeemAmount;
  
  // Burn issued token from user

• _burn(_msgSender(), _depositAmount);

• _burn(holder, _depositAmount);

  // Process the protocol fee. We can re-mint some of the burned tokens, since we aren't paying out the backing collateral
  _processProtocolFeeViaMinting(issuanceTreasury, protocolFeeAmount);
  
  // Cache Collateral Token
  IERC20 collateralToken = __Module_orchestrator.fundingManager().token();
  
  // Require that enough collateral token is held to be redeemable
  if (
      (collateralRedeemAmount + projectCollateralFeeCollected)
          > collateralToken.balanceOf(address(this))
  ) {
      revert
          Module__RedeemingBondingCurveBase__InsufficientCollateralForRedemption(
      );
  }
  
  // Get net amount, protocol and workflow fee amounts
  (collateralRedeemAmount, protocolFeeAmount, workflowFeeAmount) =
  _calculateNetAndSplitFees(
      collateralRedeemAmount, collateralSellFeePercentage, sellFee
  );
  // Process the protocol fee
  _processProtocolFeeViaTransfer(
      collateralTreasury, collateralToken, protocolFeeAmount
  );
  
  // Add workflow fee if applicable
  if (workflowFeeAmount > 0) {
      projectCollateralFeeCollected += workflowFeeAmount;
  } // Add fee amount to total collected fee
  
  // Revert when the redeem amount is lower than minimum amount the user expects
  if (collateralRedeemAmount < _minAmountOut) {
      revert Module__BondingCurveBase__InsufficientOutputAmount();
  }
  // Transfer tokens to receiver
  collateralToken.transfer(_receiver, collateralRedeemAmount); //@audit blocklist tokens
  // Emit event
  emit TokensSold(
      _receiver, _depositAmount, collateralRedeemAmount, _msgSender()
  );

  }

The mitigation is as simple as letting users provide a different destination of their choosing. Then set the tokens to be burned from the hardcoded msg.sender, but the collateral to be sent to the given destination. In our case we add a holder=msg.sender and keep the receiver as a standalone variable.

[0xRizwan]

In such case, User can directly call sellFor() which has _receiver param.

    function sellFor(address _receiver, uint _depositAmount, uint _minAmountOut)

Issuance tokens will be burned from sellFor() function caller:

221        // Burn issued token from user
222        _burn(_msgSender(), _depositAmount);

and the passed receiver in sellFor() can get the collateral/USDC/USDT.

259         // Transfer tokens to receiver
260        collateralToken.transfer(_receiver, collateralRedeemAmount);

Correct me, if i am missing something

[PlamenTSV]

Correct, but it deems the regular sell function useless, thus the severity.

[0xRizwan]

Its actually not usless. sell() is for self transferandsellFor()` is desired destination address transfer. Its intended functionality for users.

[PlamenTSV]

I just showcase how said functionality doesn't work in an edge-case scenario.

[FHieser]

I would say invalid, as sellFor() allows for targeted address

[0xmahdirostami]

@FHieser @0xRizwan thanks, yes its invalid