shogoki - Possible loss of Funds - Liquidation transfers may silently fail

[sherlock-admin]

shogoki

high

Possible loss of Funds - Liquidation transfers may silently fail

Summary

Transfers at Liquidation may silently fail, causing collateral be paid out withoud debt being paid, or liquidator not getting collateral.

Vulnerability Detail

in D3VaultLiquidation.sol:liquidate the caller pays the outstanding debt, and received the collateral tokens with a discount in exchange.
However, the transfer of the tokens to pay the debt, as well as the transfer of the collateral Tokens are using the transferFrom function of the ERC20 interface instead of safeTransferFrom (which is used in other functions).
Moreover, the return value of this function is not checked. As not all ERC20 tokens revert on a failed transfer, this could lead to a silent failure of a transfer. As the function will go on in this case this could lead to the liquidation to finish with either:

• The debt not being paid, but the collateral still paid to the caller --> Loss of protocol funds!
• The debt being repaid by the caller, but the collateral is not transferred --> Loss of user funds!

Impact

Possible loss of user or protocol funds.

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L55

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L59

Tool used

Manual Review

Recommendation

Usage of safeTransferFrom as in other functions of the same contract is recommended.

[Shogoki]

Escalate for 10USDC
This is not a duplicate of #203, which talks about USDC and Approval Race Condition protected tokens.
Furthermore this is a separate issue together with duplicates like: #5 #42 #64 & #214

[sherlock-admin2]

Escalate for 10USDC
This is not a duplicate of #203, which talks about USDC and Approval Race Condition protected tokens.
Furthermore this is a separate issue together with duplicates like: #5 #42 #64 & #214

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xffff11]

Agree with escalation, it was wrongly duped.

[jesusrod15]

I agree that all the issues that came about race approvals were wrongly duped. Likewise, the rest of the issues that reported incorrect use of transferfrom instead of safetrasnferfrom were always duplicated in the same way in other contest regardless that whats happens. So I think this is a duplicate x example of #11 #34 #5 #42 #88 and others issue as this, this is not duplicate of #2 #35 and others issue as this that are duplicate of #203

[hrishibhat]

Result:
Medium
Has duplicates
The duplication has been changed accordingly

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Shogoki: accepted

[H4LITUS]

This is a duplicate of #214. So why was the status of 214 changed from reward to non-reward?

[Proxy1967]

#151 is a dup of this issue yet the status was changed from reward to non-reward and to low/info. Why ? @hrishibhat

[hrishibhat]

@kirinfilip I don't think #151 clearly describes any issue. Please add a little more detail in the future from the context of the code in the submissions.

[Proxy1967]

@hrishibhat understandable, thanks for the explanation.

[Attens1423]

fix pr: https://github.com/DODOEX/new-dodo-v3/pull/30

[IAm0x52]

Fix looks good.