code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The CashManager contract does not support transfer-tax tokens accounting, which can lead to unfair distribution of cash

Proof of Concept

Currently, the CashManager contract does not implement the before-after pattern for accounting mintRequestsPerEpoch. Especially for transfer tax tokens with a variable tax this can lead to issues where users which are depositing in times of lower taxes will receive the same increase in mintRequestsPerEpoch as users for higher taxes. Moreover this will lead to refund issues because the assetSender might have less tokens than desired.

*We acknowledge that, for now, this contract will just use USDC as collateral. However, in our opinion this issue should still be solved for the future or for potential forks. Due to that fact, the issue will just be rated as medium instead of high.

Tools Used

VSCode

Recommended Mitigation Steps

Consider using the known before-after pattern to calculate collateralAmountMin:

uint before = collateral.balanceOf(assetRecipient);
collateral.safeTransferFrom(msg.sender, assetRecipient, depositValueAfterFees);
collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);
uint after = collateral.balanceOf(assetRecipient);
uint realized = after - before;
mintRequestsPerEpoch[currentEpoch][msg.sender] += realized;

That might be one solution to account properly for tokens with a transfer-tax.