code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L403 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L126 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L139

Vulnerability details

Impact

Malicious allowance spenders will be able to spend more than the protocol intended.

Proof of Concept

When modifying the approved allowance of a spender address there is an issue with the spender address front-running the modification transaction and spending it before getting more tokens with the new allowance. This allows such a malicious spender to spend more than intended by the protocol.

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);
    // @audit race condition approve vuln
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a parameter with the current allowance and checking whether it matches the one at the time of executing the transaction.

Here is an example of how a fix can be implemented:

function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount,
        uint256 _currentAllowance
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_token != address(0), 17);
    _validate(_spender != address(0), 17);
    _validate(_amount > 0, 17);
    // @audit race condition approve vuln
        require(_currentAllowance == IERC20WithBurn(_token).allowance(address(this), _spender), "Race approval attack");
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Assessed type

Other

c4-pre-sort commented 1 year ago

bytes032 marked the issue as duplicate of #1500

c4-pre-sort commented 1 year ago

bytes032 marked the issue as low quality report

c4-judge commented 11 months ago

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity

GalloDaSballo commented 11 months ago

Losing my last few hairs