pashov - Code does not handle ERC20 tokens with special `transfer` implementation

[sherlock-admin]

pashov

medium

Code does not handle ERC20 tokens with special transfer implementation

Summary

Calls to ERC20::transfer method should always be checked

Vulnerability Detail

Some ERC20 tokens do not revert on failure in transfer but instead return false as a return value (for example ZRX). Because of this it has become a common practice to use OpenZeppelin's SafeERC20 to handle such weird tokens. If transfer fails, but does not revert it can leave tokens stuck in the contract - for example in eject in AutoRoller we have such a non-checked transfer, but if it failed the tokens would get stuck, before the shares used for eject were already burned.

Impact

The impact is potentially permanently lost (stuck) value for users of the protocol, but it needs a special ERC20 token to be used as underlying or to be sent in contract by mistake, hence Medium severity.

Code Snippet

https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L656 https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L659 https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L715

Tool used

Manual Review

Recommendation

Use OpenZeppelin's SafeERC20 library to handle such tokens

[jparklev]

We will add the safe transfer functions to the remaining locations

[jparklev]

Fix: https://github.com/sense-finance/auto-roller/pull/16

[aktech297]

The fix is using safeTransfer from solmate/utils/SafeTransferLib.sol (asset, coin) As a suggestion, following transfer also can be updated with safeTransfer Line - 654,656.