tsvetanovv - Unsafe ERC20.transfer()

[sherlock-admin]

tsvetanovv

medium

Unsafe ERC20.transfer()

Summary

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Vulnerability Detail

Using unsafe ERC20 methods can revert the transaction for certain tokens.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value.

Code Snippet

Periphery.sol

419: ERC20(divider.pt(adapter, maturity)).transfer(receiver, uBal);
420: ERC20(divider.yt(adapter, maturity)).transfer(receiver, uBal);
744: if (ptBal > 0) ERC20(pt).transfer(receiver, ptBal)

Tool used

Manual Review

Recommendation

Recommend using OpenZeppelin's SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

[jparklev]

Disputed: We only use transfer() on PTs and YTs, which are tokens that we know do not need the safeApprove() function.

[cvetanovv]

Escalate for 10 usdc.

Just as I wrote in the report, the bool value is not checked if it is true or false after the transfer.

You can see that the token YT transfer returns a bool value: https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/tokens/YT.sol

[sherlock-admin]

Escalate for 10 usdc.

Just as I wrote in the report, the bool value is not checked if it is true or false after the transfer.

You can see that the token YT transfer returns a bool value: https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/tokens/YT.sol

You've created a valid escalation for 10 USDC!

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.

[spyrosonic10]

Escalate for 10 USDC

This issue is invalid .
YT and PT extends Solmate ERC20 which uses Solc 0.8.x. During transfer or transferFrom If the balance subtraction fail then whole transaction will fail and never return false. Hence YT and PT token do not need to use safe transfer or check returned bool value.

[sherlock-admin]

Escalate for 10 USDC

This issue is invalid .
YT and PT extends Solmate ERC20 which uses Solc 0.8.x. During transfer or transferFrom If the balance subtraction fail then whole transaction will fail and never return false. Hence YT and PT token do not need to use safe transfer or check returned bool value.

You've created a valid escalation for 10 USDC!

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.

[hrishibhat]

Escalation accepted

Not a valid issue Accepting the 2nd Escalation as the token never returns false in the solmate ERC20

[sherlock-admin]

Escalation accepted

Not a valid issue Accepting the 2nd Escalation as the token never returns false in the solmate ERC20

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.