code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

For the initial launch, the CashManager contracts will use USDC as the only collateral option. If they later decide to launch CashManagers with other forms of collateral they will be susceptible vulnerabilities due to the fee on transfer that can be turned on in USDT and other tokens.

Proof of Concept

If Ondo decides to launch a CashManager with USDT or another token with a fee on transfer feature requestMin() and 'completeRedemption' would either fail if the exact amount was approved or it could end up consuming more collateral than expected.

For requestMin() it is assumed that feesInCollateral + depositValueAfterFees = colalteralAmountIn, which is not true if a fee is taken on transfer. The second transfer to assetRecipient will either fail or consume more than expected.

The consequences for completeRedemption() are similar, when the second transferFrom() to feeRecipient is executed either it fails or more than expected.

In both these scenarios either the function fails if the exact amount was approved or more than the expected amount is used. This could lead to a scenario where less than the expected amount of collateral is held by Ondo if the issue is not found early. Both users and Ondo would hold fewer collateral tokens than expected. The consequences due to completeRedemption() paying out more collateral than expected could jeopardize the Cash token since less collateral than expected is held and because the CashManager contract can't be upgraded to fix the issue.

Tools Used

Manual review

Recommended Mitigation Steps

Check if the collateral token has a basisPointsRate > 0 if it has it should be taken into account when transferring collateral.

For full assurance that a token does not take a fee, the balance of each recipient can be checked before and after to be certain that no fee is taken by the token.

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

trust1995 marked the issue as grade-b

c4-sponsor commented 1 year ago

tom2o17 marked the issue as sponsor disputed