Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecated safeApprove() function #214

Closed
gzeoneth opened this issue May 7, 2022 · 1 comment
Closed

Deprecated safeApprove() function #214

gzeoneth opened this issue May 7, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists

Comments

@gzeoneth
Copy link
Member

gzeoneth commented May 7, 2022

Originally submitted by warden Dravee in #146, duplicate of #178 related to the use of safeApprove.
This is upgraded from a QA report to standalone issue because it correctly described the revert when trying to call safeApprove on non-zero allowance. QA report that only describe safeApprove as deprecated and unable to identify the revert problem are intentionally not upgraded. Duplicate of #180.

Deprecated safeApprove() function

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). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

backd/contracts/CvxCrvRewardsLocker.sol:
  53:         IERC20(CRV).safeApprove(CRV_DEPOSITOR, type(uint256).max);
  56:         IERC20(CVX_CRV).safeApprove(CVX_CRV_STAKING, type(uint256).max);
  59:         IERC20(CRV).safeApprove(CVX_CRV_CRV_CURVE_POOL, type(uint256).max);
  62:         IERC20(CVX).safeApprove(CVX_LOCKER, type(uint256).max);

backd/contracts/actions/topup/TopUpAction.sol:
   50:             IERC20(token).safeApprove(stakerVaultAddress, depositAmount);
  847:         IERC20(depositToken).safeApprove(feeHandler, feeAmount);
  908:         IERC20(token).safeApprove(spender, type(uint256).max);

backd/contracts/actions/topup/handlers/AaveHandler.sol:
  53:         IERC20(underlying).safeApprove(address(lendingPool), amount);

backd/contracts/actions/topup/handlers/CompoundHandler.sol:
   71:             IERC20(underlying).safeApprove(address(ctoken), amount);
  120:             IERC20(underlying).safeApprove(address(ctoken), debt);

backd/contracts/pool/LiquidityPool.sol:
  721:         IERC20(lpToken_).safeApprove(staker_, type(uint256).max);

backd/contracts/strategies/BkdEthCvx.sol:
  43:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);

backd/contracts/strategies/BkdTriHopCvx.sol:
   71:         IERC20(underlying_).safeApprove(curveHopPool_, type(uint256).max);
   72:         IERC20(hopLp_).safeApprove(curvePool_, type(uint256).max);
   73:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);
  129:         IERC20(hopLp).safeApprove(curvePool_, 0);
  130:         IERC20(hopLp).safeApprove(curvePool_, type(uint256).max);
  131:         IERC20(lp_).safeApprove(address(_BOOSTER), 0);
  132:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);

backd/contracts/strategies/ConvexStrategyBase.sol:
  107:         _CRV.safeApprove(address(_strategySwapper), type(uint256).max);
  108:         _CVX.safeApprove(address(_strategySwapper), type(uint256).max);
  109:         _WETH.safeApprove(address(_strategySwapper), type(uint256).max);
  279:         IERC20(token_).safeApprove(address(_strategySwapper), 0);
  280:         IERC20(token_).safeApprove(address(_strategySwapper), type(uint256).max);

backd/contracts/strategies/StrategySwapper.sol:
  209:         IERC20(token_).safeApprove(spender_, type(uint256).max);

backd/contracts/vault/Erc20Vault.sol:
  21:         IERC20(underlying_).safeApprove(address(reserve), type(uint256).max);
  22:         IERC20(underlying_).safeApprove(_pool, type(uint256).max);
@gzeoneth gzeoneth added bug Something isn't working duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 7, 2022
@gzeoneth gzeoneth closed this as completed May 7, 2022
JeeberC4 added a commit that referenced this issue May 13, 2022
@JeeberC4
Copy link
Contributor

Manually created required json file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants