Approving from non-zero to non-zero allowance will revert with OZ's `safeApprove()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/RewardHandler.sol#L52

Vulnerability details

Impact

Transaction reverting.

Proof of Concept

OZ's implementation of safeApprove would throw an error if an approve is attempted from a non-zero value ("SafeERC20: approve from non-zero to non-zero allowance"): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L53-L56

    require(
        (value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );

Affected Code

See @audit tag:

File: RewardHandler.sol
35:     function burnFees() external override {
...
50:         feeBurner.burnToTarget{value: ethBalance}(tokens, targetLpToken);
51:         uint256 burnedAmount = IERC20(targetLpToken).balanceOf(address(this));
52:         IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount); //@audit this would revert on the 2nd use if balance is not 0
53:         bkdLocker.depositFees(burnedAmount);
54:         emit Burned(targetLpToken, burnedAmount);
55:     }

Mitigations

Set the allowance to zero immediately before this safeApprove() call:

File: RewardHandler.sol
    function burnFees() external override {
...
+        IERC20(targetLpToken).safeApprove(address(bkdLocker), 0); //@audit-ok : approving 0 first
        IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount);  //@audit-ok : approving burnedAmount second
...
    }

[chase-manning]

Probably better to use infinite approval here to save on gas also 😃

[GalloDaSballo]

The warden asserts that the code will revert if allowance is non-zero, that is indeed a property of safeApprove

However, no attempt is made at showing any situation in which allowance will be non-zero to non-zero.

The code for depositFees is available and shows that the transfer will be for the full amount.

In lack of a demonstration of the code reverting I think the finding is invalid.