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.
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 (https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219).
Proof Of Concept
Refer to the
burnFees
function on line 35 of theRewardHandler
contract (https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L35-L55).Pool underlying tokens do correctly call the internal
_approve
function present in theRewardHandler
contract that performs an allowance check before callingsafeApprove
(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).Tools Used
vim
Recommended Mitigation Steps
One potential mitigation would be to replace usage of
safeApprove()
withsafeIncreaseAllowance()
orsafeDecreaseAllowance()
instead. However, in the context of theRewardHandler.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.