Lack of support for fee-on-transfer token #51

code423n4 · 2023-01-09T02:37:45Z

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/TransferProxy.sol#L34
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L181
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L643

Vulnerability details

Impact

Lack of support for fee-on-transfer token.

Proof of Concept

In the codebase, the usage of safeTransfer and safeTransferFrom assume that the receiver receive the exact transferred amount.

src\AstariaRouter.sol:
  528      ERC20(IAstariaVaultBase(commitments[0].lienRequest.strategy.vault).asset())
  529:       .safeTransfer(msg.sender, totalBorrowed);
  530    }

src\ClearingHouse.sol:
  142  
  143:     ERC20(paymentToken).safeTransfer(
  144        s.auctionStack.liquidator,

  160      if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
  161:       ERC20(paymentToken).safeTransfer(
  162          ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),

src\PublicVault.sol:
  383  
  384:       ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);
  385        WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(

src\VaultImplementation.sol:
  393      payout = _handleProtocolFee(c.lienRequest.amount);
  394:     ERC20(asset()).safeTransfer(receiver, payout);
  395    }

  405        }
  406:       ERC20(asset()).safeTransfer(feeTo, fee);
  407      }

src\WithdrawProxy.sol:
  268      if (s.withdrawRatio == uint256(0)) {
  269:       ERC20(asset()).safeTransfer(VAULT(), balance);
  270      } else {

  280        if (balance > 0) {
  281:         ERC20(asset()).safeTransfer(VAULT(), balance);
  282        }

  297      }
  298:     ERC20(asset()).safeTransfer(withdrawProxy, amount);
  299      return amount;

However, according to https://github.com/d-xo/weird-erc20#fee-on-transfer

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

So the recipient address may not receive the full transfered amount, which can break the protocol's accounting and revert transaction.

The same safeTransfer and safeTransferFrom is used in the vault deposit / withdraw / mint / withdraw function.

Let us see a concrete example,

contract TransferProxy is Auth, ITransferProxy {
  using SafeTransferLib for ERC20;

  constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) {
    //only constructor we care about is  Auth
  }

  function tokenTransferFrom(
    address token,
    address from,
    address to,
    uint256 amount
  ) external requiresAuth {
    ERC20(token).safeTransferFrom(from, to, amount);
  }
}

the transfer Proxy also use

ERC20(token).safeTransferFrom(from, to, amount);

this transfer is used extensively

src\AstariaRouter.sol:
  208      RouterStorage storage s = _loadRouterSlot();
  209:     s.TRANSFER_PROXY.tokenTransferFrom(
  210        address(token),

src\LienToken.sol:
  184      );
  185:     s.TRANSFER_PROXY.tokenTransferFrom(
  186        params.encumber.stack[params.position].lien.token,

  654      if (payment > 0)
  655:       s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);
  656  

  860  
  861:     s.TRANSFER_PROXY.tokenTransferFrom(stack.lien.token, payer, payee, amount);
  862  

src\scripts\deployments\Deploy.sol:
  378        uint8(UserRoles.ASTARIA_ROUTER),
  379:       TRANSFER_PROXY.tokenTransferFrom.selector,
  380        true

  403        uint8(UserRoles.LIEN_TOKEN),
  404:       TRANSFER_PROXY.tokenTransferFrom.selector,
  405        true

If the token used charge transfer fee, the accounting below is broken:

When _payDebt is called

  function _payDebt(
    LienStorage storage s,
    address token,
    uint256 payment,
    address payer,
    AuctionStack[] memory stack
  ) internal returns (uint256 totalSpent) {
    uint256 i;
    for (; i < stack.length;) {
      uint256 spent;
      unchecked {
        spent = _paymentAH(s, token, stack, i, payment, payer);
        totalSpent  = spent;
        payment -= spent;
          i;
      }
    }
  }

which calls _paymentAH

  function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] memory stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
    uint256 lienId = stack[position].lienId;
    uint256 end = stack[position].end;
    uint256 owing = stack[position].amountOwed;
    //checks the lien exists
    address payee = _getPayee(s, lienId);
    uint256 remaining = 0;
    if (owing > payment.safeCastTo88()) {
      remaining = owing - payment;
    } else {
      payment = owing;
    }
    if (payment > 0)
      s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

    delete s.lienMeta[lienId]; //full delete
    delete stack[position];
    _burn(lienId);

    if (_isPublicVault(s, payee)) {
      IPublicVault(payee).updateAfterLiquidationPayment(
        IPublicVault.LiquidationPaymentParams({remaining: remaining})
      );
    }
    emit Payment(lienId, payment);
    return payment;
  }

not that the code

s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

if the token charge transfer fee, for example, the payment amout is 100 ETH, 1 ETH is charged as fee, the recipient only receive 99 ETH,

but the wrong value payment 100 ETH is returned and used to update the accounting

unchecked {
	spent = _paymentAH(s, token, stack, i, payment, payer);
	totalSpent  = spent;
	payment -= spent;
    i;
}

then the variable totalSpent and payment amoutn will be not valid.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol whitelist the token address or use balance before and after check to make sure the recipient receive the accurate amount of token when token transfer is performed.

c4-judge · 2023-01-23T12:09:32Z

Picodes marked the issue as primary issue

c4-sponsor · 2023-02-02T00:44:12Z

SantiagoGregory marked the issue as sponsor acknowledged

SantiagoGregory · 2023-02-02T00:44:40Z

USDC and USDT fees would break other contracts as well, and we won't be supporting other tokens with fees at a UI level. @androolloyd

c4-judge · 2023-02-23T11:50:05Z

Picodes marked the issue as satisfactory

c4-judge · 2023-02-23T11:50:11Z

Picodes marked the issue as selected for report

code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 9, 2023

code423n4 added a commit that referenced this issue Jan 9, 2023

ladboy233 issue #51

6e21aff

c4-judge added the primary issue Highest quality submission among a set of duplicates label Jan 23, 2023

c4-sponsor added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Feb 2, 2023

c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 23, 2023

c4-judge added the selected for report This submission will be included/highlighted in the audit report label Feb 23, 2023

C4-Staff added the M-33 label Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lack of support for fee-on-transfer token #51

Lack of support for fee-on-transfer token #51

code423n4 commented Jan 9, 2023

c4-judge commented Jan 23, 2023

c4-sponsor commented Feb 2, 2023

SantiagoGregory commented Feb 2, 2023 •

edited

Loading

c4-judge commented Feb 23, 2023

c4-judge commented Feb 23, 2023

Lack of support for fee-on-transfer token #51

Lack of support for fee-on-transfer token #51

Comments

code423n4 commented Jan 9, 2023

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

c4-judge commented Jan 23, 2023

c4-sponsor commented Feb 2, 2023

SantiagoGregory commented Feb 2, 2023 • edited Loading

c4-judge commented Feb 23, 2023

c4-judge commented Feb 23, 2023

SantiagoGregory commented Feb 2, 2023 •

edited

Loading