Ondo Finance contest - Tajobin's results

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/kyc/KYCRegistry.sol#L82

Vulnerability details

Impact

There is no time limit on the validity off KYC digests and users with a removed KYC are not saved. If a issuer of such a digest is either compromised or if they by mistake issue a digest with a deadline far into the future a user could reuse the same digest to acquire KYC status after it has been removed.

Even if no mistake has been made a user could attempt to attack the protocol by quickly purposely losing KYC by becoming a sanctioned entity and then reusing the same signature to acquire KYC even though they are a known sanctioned entity.

Proof of Concept

I will outline 2 different scenarios:

1. If deadline is set to a date far into the future, either by mistake or compromised digest issuer, a user could continuously re-KYC themselves by using the same digest. Depending on how Ondo records KYCd users this could lead to a mismatch between their off-chain records and the actual KYCd users. Even if Ondo automatically checks that their off-chain records match the events emitted a user could potentially re-kyc every time they wish to use the protocol. As long as Ondo does not realize what it going on and removes the permission of the signer of the digest the user could re-KYC.

2. Even if no mistake is made and issuer is not compromised a malicious actor could stage an attack on Ondo. If deadline is set to a reasonable number time an attack could KYC with a stolen identity and a clean address. After passing KYC the user starts receiving a large amount of assets from Tornado Cash, the user is now holding sanctioned funds and could potentially be considered a sanctioned entity. Ondo would reasonably run off-chain checks on their KYCd users to check on their status, such a user could be removed before the deadline and re-KYC and use the protocol. Ondo would then have allowed a known sanctioned entity that had its KYC removed to use its protocol. This could have both reputation and legal consequences.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the kycState mapping to map to an enum instead bool with 3 different states. NON-KYC, KYC and REMOVED-KYC and to add a check in addKYCAddressViaSignature() to check that the user does not have the NON-KYC status. To re-KYC a user the manager would have to first change the status from REMOVED-KYC to NON-KYC it could then apply for KYC as usual.

Adding a time limit to how long a digest is valid is also a good idea to have certainty in that there aren't users out there that could KYC themselves far into the future. Add additional variables, issue date and end date. Check that those are within certain limits.

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.