search

yearn / yearn-protocol

Yearn smart contracts
https://yearn.finance
GNU Affero General Public License v3.0
441 stars 211 forks source link

why safeApprove two times? #113

Closed guotie closed 3 years ago

guotie commented 3 years ago 
            token.safeApprove(addr, 0);
            token.safeApprove(addr, uint256(-1));

why first safeApprove(0), then safeApprove(amount)?

yurenji commented 3 years ago

see comment : https://github.com/nachomazzara/SafeERC20/blob/master/contracts/libs/SafeERC20.sol#L78

guotie commented 3 years ago

thanks.

sinasab commented 3 years ago

It's still not super obvious to me why this is needed. The link references transaction ordering being a vulnerability, but in the case referenced in the issue both approves are happening within the scope of a single transaction.

Am I missing something? Could you perhaps elaborate on what an example attack would look like if we didn't approve to 0 before doing the other approve? Because again the one in the link doesn't seem to apply.

guotie commented 3 years ago

uniswap did not approve twice.

sinasab commented 3 years ago

Maybe reopen the issue so someone on the core team can take a look?

It looks like we're doing the double-approvals in a bunch of strategies, so not a huge issue, but ultimately may be a waste of gas if I'm understanding it correctly.

saltyfacu commented 3 years ago

It's the right way to do it with SafeApprove. If the user already has an allowance, you set the allowance to 0 and then set the allowance to max.