Permanent freeze of yield when TokenSender rewards bank is depleted and deposit or withdraw is called.

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/TokenSender.sol#L41

Vulnerability details

Description

In collateral deposit() and withdraw() flow, a fee is calculated as a percentage of user's requested amount. It is passed to the DepositHook and WithdrawHook, for example in deposit():

uint256 _amountAfterFee = _amount - _fee;
if (address(depositHook) != address(0)) {
  baseToken.approve(address(depositHook), _fee);
  // hook will calculate fee = _amount - _amountAfterFee
  depositHook.hook(_recipient, _amount, _amountAfterFee);
  baseToken.approve(address(depositHook), 0);
}

In the DepositHook, if there is a fee two actions take place:

1. Collect the fee to the treasury from Collateral contract
2. Send rewards relative to fee amount, to the depositor

uint256 _fee = _amountBeforeFee - _amountAfterFee;
if (_fee > 0) {
  collateral.getBaseToken().transferFrom(address(collateral), _treasury, _fee);
  _tokenSender.send(_sender, _fee);
}

The issue is that TokenSender has an early exit problem:

function send(address recipient, uint256 unconvertedAmount) external override onlyAllowedMsgSenders {
  uint256 scaledPrice = (_price.get() * _priceMultiplier) / MULTIPLIER_DENOMINATOR;
  if (scaledPrice <= _scaledPriceLowerBound) return;
  uint256 outputAmount = (unconvertedAmount * _outputTokenDecimalsFactor) / scaledPrice;
  if (outputAmount == 0) return;
  // EARLY EXIT PROBLEM
  if (outputAmount > _outputToken.balanceOf(address(this))) return;
  _outputToken.transfer(recipient, outputAmount);
}

In the code above, note that if the TokenSender does not currently have enough reward tokens to hand out, it will simply return successfully from the call. Therefore, user which assumed they will be getting cash back rewards from fees when depositing or withdrawing, are actually paying the fees with no compensation.

Impact

Permanent freeze of yield when TokenSender rewards bank is depleted and deposit or withdraw is called.

Proof of Concept

1. User has $1000 USDC
2. User deposits it to Collateral.sol, which charges 10% deposit fee
3. Meanwhile, TokenSender offers PPO rewards, but has just ran out.
4. DepositHook calls TokenSender.send(), which should send $100 fee rewards in PPO to user. send() returns without paying.
5. User is unhappy having assumed they will receive PPO from the fee.

Tools Used

Manual audit

Recommended Mitigation Steps

The root cause is that there is no differentiation between user's request to mint expecting rewards, and user's request to mint in any case. This lack of acknoledgement means user may be left under-compensated.

It is very advisable to add "forfeitRewards" flag, which is required to be true when TokenSender is not able to satisfy the reward owings to user.

Judging note

Permanent freezing of unclaimed yield is always rated as High severity on Immunefi bounties.

[Picodes]

Indeed, it would be good to add a flag or some parameter called minimumCompensationExpected.

However, I do not agree that this falls in the "Permanent freezing of unclaimed yield" category. From my perspective, this part of the code does not promise users anything. Instead, there is eventually a fee rebate if there are still PPO to send. Therefore, the additional parameter would only be an additional safety check for the user.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

ramenforbreakfast marked the issue as sponsor disputed

[ramenforbreakfast]

Yes, i agree that to categorize this under "Permanent freezing of unclaimed yield", since this is just an additional rebate reward given to the user. It is within the contract documentation that this is expected behavior and in our front end we will be very clear on the lack of a rebate if the TokenSender contract does not have funds to issue one.

We do not want a situation where the rebate contract has exhausted its PPO and the platform is halted because of it.

[c4-judge]

Picodes marked the issue as duplicate of #257

[c4-judge]

Picodes marked the issue as satisfactory