code-423n4 / 2023-08-dopex-findings

3 stars 3 forks source link

`approveContractToSpend` should accept `amount == 0` to cancel approval. #1354

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L410 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L133

Vulnerability details

Impact

Both RdpxV2Core and UniV2LiquidityAmo can use approveContractToSpend to approve a contract to spend a certain amount of tokens. However, it can only be used to give approval. There is no other method that can remove the given approval, leading to potential loss of funds.

Proof of Concept

approveContractToSpend can only accept _amount > 0, it cannot be used to cancel the given approval. It is dangerous that the contract cannot renounce the given approval. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L410 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L133

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_token != address(0), 17);
    _validate(_spender != address(0), 17);
    _validate(_amount > 0, 17);
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(_token != address(0), "reLPContract: token cannot be 0");
    require(_spender != address(0), "reLPContract: spender cannot be 0");
    require(_amount > 0, "reLPContract: amount must be greater than 0");
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

approveContractToSpend should accept _amount == 0 to cancel the approval.

Assessed type

Error

c4-pre-sort commented 1 year ago

bytes032 marked the issue as duplicate of #797

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)