code-423n4 / 2021-06-pooltogether-findings

0 stars 0 forks source link

`BadgerYieldSource` ERC20 return values not checked #87

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

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.

It is not checked in BadgerYieldSource.supplyTokenTo.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.

As the badger is merely a parameter to the yield source it is not known which token & Sett will end up actually being used.

Recommended Mitigation Steps

We 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.

asselstine commented 3 years ago

See https://github.com/code-423n4/2021-06-pooltogether-findings/issues/112

It's more of a 1 (Low Risk) because the subsequent deposit calls will fail. There is no advantage to be gained; the logic is simply poor.

dmvt commented 3 years ago

duplicate of #112