Call to safeApprove without checking previous allowance in burnFees could result in locked funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219).

Proof Of Concept

Refer to the burnFees function on line 35 of the RewardHandler contract (https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L35-L55).

function burnFees() external override {
    [...]
    feeBurner.burnToTarget{value: ethBalance}(tokens, targetLpToken);
    uint256 burnedAmount = IERC20(targetLpToken).balanceOf(address(this));
    IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount);
    [...]
}

Pool underlying tokens do correctly call the internal _approve function present in the RewardHandler contract that performs an allowance check before calling safeApprove (https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L46 & https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L57-L65).

function _approve(address token, address spender) internal {
    if (IERC20(token).allowance(address(this), spender) > 0) return;
    IERC20(token).safeApprove(spender, type(uint256).max);
}

Tools Used

vim

Recommended Mitigation Steps

One potential mitigation would be to replace usage of safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead. However, in the context of the RewardHandler.sol contract, it would be enough to use the internal _approve() function that has the additional check to ensure no previous allowance is outstanding, therefore preventing the locking of funds.

[chase-manning]

duplicate of #163

[GalloDaSballo]

Dup of #163