Open code423n4 opened 3 years ago
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.
Handle
shw
Vulnerability details
Impact
In the contracts
BadgerYieldSource
andSushiYieldSource
, the return values of ERC20transfer
andtransferFrom
are not checked to betrue
, which could befalse
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, thebadger
variable should be theBADGER
token at address0x3472a5a71965499acd81997a54bba8d852c6e53d
. However, this implementation ofBADGER
is not ERC20-compliant, which returnsfalse
when the sender does not have enough token to transfer (both fortransfer
andtransferFrom
). 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 callsafeTransfer
orsafeTransferFrom
when transferring ERC20 tokens.