Return values of ERC20 transfer and transferFrom are unchecked

[code423n4]

Handle

shw

Vulnerability details

Impact

In the contracts BadgerYieldSource and SushiYieldSource, the return values of ERC20 transfer and transferFrom are not checked to be true, which could be false if the transferred tokens are not ERC20-compliant (e.g., BADGER). In that case, the transfer fails without being noticed by the calling contract.

Proof of Concept

If warden's understanding of the BadgerYieldSource is correct, the badger variable should be the BADGER token at address 0x3472a5a71965499acd81997a54bba8d852c6e53d. However, this implementation of BADGER is not ERC20-compliant, which returns false when the sender does not have enough token to transfer (both for transfer and transferFrom). See the source code on Etherscan (at line 226) for more details.

Referenced code:
BadgerYieldSource.sol#L44
BadgerYieldSource.sol#L79
SushiYieldSource.sol#L48
SushiYieldSource.sol#L89

Recommended Mitigation Steps

Use the SafeERC20 library implementation from Openzeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.

[kamescg]

Sushi

• https://github.com/pooltogether/sushi-pooltogether/pull/new/fix/112
• update to support SafeERC20 pooltogether/sushi-pooltogether#11

Badger

• https://github.com/pooltogether/badger-yield-source/pull/new/fix/112
• update to support SafeERC20 pooltogether/badger-yield-source#2

[dmvt]

Sponsor has repeatedly stated in duplicate issues that: "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."

I disagree with this assessment. The function(s) in question do not immediately call deposit or another function that would cause a revert. In fact the balances are updated:

        balances[msg.sender] = balances[msg.sender].sub(requiredSharesBalance);
        badger.transfer(msg.sender, badgerBalanceDiff);
        return (badgerBalanceDiff);

The impact that this would have on the rest of the system is substantial, including causing incorrect balances to be returned and potentially lost funds.

That said, I do not think this is very likely and so high severity seems excessive here. Im adjusting all of these reports to Medium Risk given that lower likelihood.