Core Contracts

Summary

The amount param which is in underlying asset units (i.e. crvUSD) gets scaled down twice when a user calls transfer() or transferFrom() --> once inside transfer()/transferFrom() and again inside the overriden _update():

and

Description & Impact

1. Impact on both transfer functions : Reduced tokens transferred

• If liquidity index is 1.025, then a transfer of amount = 200 would result in only a transfer of 200 / (1.025 * 1.025) which is 190.362 in scaledBalance terms or 195.121 in balance terms. This should have been 195.121 in scaledBalance terms or 200 in balance terms.

2. Impact on transferFrom() : Funds can be stolen

• If transferFrom() is used, then there are additional concerns:

  • First, Alice gives Bob an allowance of 200 as she wants to transfer these many underlying asset units to Charlie

  • Bob calls transferFrom(sender = alice, recipient = bob, amount = 200)

  • transferFrom() calculates scaledAmount = 200 / 1.025 = 195.121 and internally calls super.transferFrom(sender = alice, recipient = bob, amount = 195.121)

  • This calls ERC20.sol's transferFrom() which is:

  function transferFrom(address from, address to, uint256 value) public virtual returns (bool) {

  address spender = _msgSender();

  @---> _spendAllowance(from, spender, value);

  _transfer(from, to, value);

  return true;

  }

  • Bob's allowance is decreased by 195.121 via the internal call to _spendAllowance() leaving him with an unspent allowance of 200 - 195.121 = 4.9 approx

  • Additionally, the actual transfer is handled by the overridden _update() which scales this down again and transfers 195.121 / 1.025 = 190.362 tokens.

Bob can now use his leftover 4.9 allowance to steal & transfer these tokens to an address owned by him.

• It's important to note that even if a fix is made inside RToken::transferFrom() and a call is made without scaling like so: return super.transferFrom(sender, recipient, amount);, the allowance reduction inside _spendAllowance() will reduce it by 200 but only transfer 195.121. Thus Bob won't be able to steal any tokens but can't retry a transfer now because his allowance is exhausted. Alice will have to provide additional allowance by calculating underlyingAmountToBeTransferred * liquidityIndex.

Proof of Concept

Add this inside test/unit/core/pools/LendingPool/LendingPool.test.js and run to see it pass with the following output:

Output:

Mitigation

While the simple way to mitigate this would be to remove the scaledAmount calculation from inside RToken's transfer() and transferFrom() and pass amount while calling super.transfer() and super.transferFrom(), this won't solve the allowance issue in transferFrom().
It would either require a redesign of the approach so that it's easier for the owner to understand the allowance they should be giving the spender, or block the transferFrom() functionality altogether.