Why do ERC20Helper functions not revert on failure?

snd commented 2 years ago

As opposed to their OpenZeppelin counterparts the functions in ERC20Helper don't revert on failure.

This makes them harder to use correctly. Developers MUST wrap every call to a function in ERC20Helper in a require statement. Forgetting to do so could introduce critical issues.

Is there a specific reason why this behavior was chosen?

Without further context, we'd highly recommend making ERC20Helpers functions revert on failure.

The first transfer in the Liquidator is not wrapped in a require: https://github.com/maple-labs/liquidations/blob/35c628e5ab45fbffaab7aef43a030a98b712a94a/contracts/Liquidator.sol#L42 Is this intentional?

At first glance this seems to enable the following scenario:

user calls Liquidator.liquidatePortion(swapAmount, "")
collateralAsset.balance(Liquidator) < swapAmount i.e. the liquidator has not enough collateral left
ERC20Helper.transfer(collateralAsset, msg.sender, swapAmount_) returns false i.e. fails silently
the user pays for the collateral but doesn't get it i.e. the Liquidator "steals" funds from the user

If confirmed this would be a high severity finding.

deluca-mike commented 2 years ago

Yes, there is a specific reason. Unlike OpenZeppelin contracts, which are relatively highly opinionated (and thus can carry bloat and inflexibility), ours are intended to be used by "advanced devs", and thus give them more flexibility, which includes the flexibility to shoot yourself in the foot.

Reasons the library does not revert:

Contracts can choose their own, potential domained, revert strings, or none at all
Contracts make want to use them as bools (i.e. require(ERC20Helper.transfer(asset1, msg.sender, amount) || ERC20Helper.transfer(asset2, msg.sender, amount));
Contracts can not care about the result of a transfer (as we did in this specific case)

Now, specifically in liquidatePortion, we intentionally don't revert since the only thing that the liquidation contact cares is that the require line's condition is met. If a transfer of tokens to the caller fails, but the contract still gains funds it expects, then that's all the contract cares about.

So, again, we'd be ok with the Liquidator "stealing" funds from the user, because the Liquidator only cares that it is satisfied.

Now, as I am thinking about it, it might make callers feel more at ease to call liquidatePortion if the check was done, since the contract would check for them that collateralAsset was properly sent to them. I'd argue that that check should happen on their end, in the msg.sender.call(data_);, but then the argument can be made that EOAs don't have this luxury. So, in light of that, I think it does make sense to wrap and revert on that transfer.