0x52 - Protocol is completely incompatible with USDT due to lack of 0 approval

[sherlock-admin]

0x52

medium

Protocol is completely incompatible with USDT due to lack of 0 approval

Summary

USDT will revert if the current allowance is greater than 0 and an non-zero approval is made. There are multiple instances throughout the contracts where this causes issues. In some places this can create scenarios where it becomes impossible to liquidate and/or borrow it.

Vulnerability Detail

See summary.

Impact

USDT may become impossible to liquidate or borrow

Code Snippet

D3Funding.sol#L20-L23

D3Funding.sol#L50-L53

D3Funding.sol#L64-L67

D3MMLiquidationRouter.sol#L24

Tool used

Manual Review

Recommendation

Utilize the OZ safeERC20 library and safeApprove

[osmanozdemir1]

Escalate for 10 USDC

I agree with "approve to 0 first" is one of the most common bugs in the space but I think it is invalid for this protocol.

Approval calls to token contracts are made from pool contracts in this protocol, like borrow() and updateReserveByVault() functions.

Before calling the approve function, the allowance of the vault is checked and the call is made if allowance < type(uint256).max. Then the approve is called and the allowance is set to type(uint256).max.

• If a pool tries to borrow for the first time, allowance is set to type(uint256).max.
• Allowance will not decrease after that ever even if the vault uses that allowance. The reason for that is according to ERC20, allowances will not decrease after spending if the allowance was type(uint256).max.
• If the pool tries to borrow again, there won't be a problem as allowance is already type(uint256).max, and this call will not be made.

I believe the issue is invalid for this protocol as the approval is not expected to be used by EOAs. A pool can not directly approve to a non-zero value because it doesn't have a function to make that call. That's why I don't see a scenario where the allowance is not zero, and not type(uint256).max at the same time. It has to be either 0 or type(uint256).max, and this won't cause function to revert.

[sherlock-admin2]

Escalate for 10 USDC

I agree with "approve to 0 first" is one of the most common bugs in the space but I think it is invalid for this protocol.

Approval calls to token contracts are made from pool contracts in this protocol, like borrow() and updateReserveByVault() functions.

Before calling the approve function, the allowance of the vault is checked and the call is made if allowance < type(uint256).max. Then the approve is called and the allowance is set to type(uint256).max.

• If a pool tries to borrow for the first time, allowance is set to type(uint256).max.
• Allowance will not decrease after that ever even if the vault uses that allowance. The reason for that is according to ERC20, allowances will not decrease after spending if the allowance was type(uint256).max.
• If the pool tries to borrow again, there won't be a problem as allowance is already type(uint256).max, and this call will not be made.

I believe the issue is invalid for this protocol as the approval is not expected to be used by EOAs. A pool can not directly approve to a non-zero value because it doesn't have a function to make that call. That's why I don't see a scenario where the allowance is not zero, and not type(uint256).max at the same time. It has to be either 0 or type(uint256).max, and this won't cause function to revert.

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.

[0xSulpiride]

@osmanozdemir1 isn't the allowance check done wrong in the line 50?
It should be allowance(address(this), state._D3_VAULT_), otherwise it will call approve on each call of updateReserveByVault, so I think the issue is valid.

[osmanozdemir1]

@osmanozdemir1 isn't the allowance check done wrong in the line 50? It should be allowance(address(this), state._D3_VAULT_), otherwise it will call approve on each call of updateReserveByVault, so I think the issue is valid.

Oh, I see. You're definitely right about that, and this makes it problematic due to the wrong implementation in line 50. Root cause is wrong allowance check and I totally missed it. Thanks for the comment.

[hrishibhat]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• osmanozdemir1: rejected

[traceurl]

fixed in https://github.com/DODOEX/new-dodo-v3/pull/30

[IAm0x52]

Fix looks good