search

code-423n4 / 2023-01-ondo-findings

0 stars 0 forks source link

Redeemers can reject receiving air-drop collateral token, make function `completeRedemptions()` failing and wasting gas of admin #316

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L707

Vulnerability details

Impact

Function completeRedemptions() is used by admin account to distribute collateral to users and also to refund redemption requests if the redemption cannot be serviced.

Redeemers will received funds from assetSender in air-drop manner, if collateral token is ERC777-like token with a callback to receiver, redeemers can reject receiving collateral token back, make the whole funcion completeRedemptions() failing.

In addition, function completeRedemptions() is usually called with a large number of redeemers, refundees at once. So the gas cost is extremely high, if the attacker is at the end of the list and he reject the transfer, it will cost admin a lot of money for gas cost.

Proof of Concept

Consider the scenario where collateral token is token with hook to receiver.

  1. There are 1000 users send redemption request in one epoch. 1000th one is attacker and he only withdraw a tiny amount.
  2. When admin call completeRedemptions(), after done transfering to 999 redeemers, the 1000th is a contract that reject receiving collateral back.
  3. The whole transaction will fail.

Obviously, admin can easily punish attacker by removing him from next call (i.e: only call with 999 first redeemers). However, since attacker loss is so small, he can create a lot of accounts and it will be really difficult for admin to eliminate all of them at once.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider applying Pull over Push pattern, allowing redeemers to claim their funds later to avoid being DOS.

c4-judge commented 1 year ago

trust1995 marked the issue as primary issue

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid

trust1995 commented 1 year ago

collateral is USDC, so should not be susceptible to the attack.

tom2o17 commented 1 year ago

You will have to kill me before we let any ERC-20-like token with hooks be used as collateral w/n cashManager.