`Zapper` should safeApprove(0) first

[code423n4]

Handle

cmichel

Vulnerability details

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).safeApprove(address(operator), 0);
IERC20(token).safeApprove(address(operator), amount);

The Zapper.approve/approveMaxMany function approves the spender without resetting it to zero first.

Impact

When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

[Ivshti]

changing this to "sponsor acknowledged" and won't fix

The reason is that we can always use Zapper.approve to zero approve such a token beforehand. Furthermore, approveMaxMany and approve are generally intended to only ever be used once per token, as realistically the approved amount will never run out when we set it to max.

if we want to un-approve, then that's a different story, but we can still use a separate "approve" call to granularly control this

the zapper is generally intended to approve tokens once and forever though

[GalloDaSballo]

Agree with finding and agree with sponsor mitigation, if this were ever to cause issues, you can just approve(0) as said