In strategistBootyClaim() of BathPair there is no check that asset!=quote and strategist can claim one asset rewards two times

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L588-L625

Vulnerability details

Impact

function strategistBootyClaim() is calculating and transferring strategist prize, but strategist can send asset=quote and claim one token prize two times as in code it gets strategist2Fills of asset and quote in the beginning of function.

Proof of Concept

This is strategistBootyClaim() code:

function strategistBootyClaim(address asset, address quote)
        external
        onlyApprovedStrategist(msg.sender)
    {
        uint256 fillCountA = strategist2Fills[msg.sender][asset];
        uint256 fillCountQ = strategist2Fills[msg.sender][quote];
        if (fillCountA > 0) {
            uint256 booty = (
                fillCountA.mul(IERC20(asset).balanceOf(address(this)))
            ).div(totalFillsPerAsset[asset]);
            IERC20(asset).transfer(msg.sender, booty);
            emit LogStrategistRewardClaim(
                msg.sender,
                asset,
                booty,
                block.timestamp
            );
            totalFillsPerAsset[asset] -= fillCountA;
            strategist2Fills[msg.sender][asset] -= fillCountA;
        }
        if (fillCountQ > 0) {
            uint256 booty = (
                fillCountQ.mul(IERC20(quote).balanceOf(address(this)))
            ).div(totalFillsPerAsset[quote]);
            IERC20(quote).transfer(msg.sender, booty);
            emit LogStrategistRewardClaim(
                msg.sender,
                quote,
                booty,
                block.timestamp
            );
            totalFillsPerAsset[quote] -= fillCountQ;
            strategist2Fills[msg.sender][quote] -= fillCountQ;
        }
    }

As you can see it first save the values of strategist2Fills[msg.sender][asset] and strategist2Fills[msg.sender][quote] and then calculated asset and quote prize for strategist and in the process set the values of strategist2Fills[msg.sender][asset] and strategist2Fills[msg.sender][quote]. if strategist send asset=quote then code will reward strategist for that token two times because uint256 fillCountQ = strategist2Fills[msg.sender][quote]; is before calculating and updating states for asset.
Of course because of strategist2Fills[msg.sender][quote] -= fillCountQ; it may end with underflow error if contract uses SafeMath correctly but the logic is wrong. the line uint256 fillCountQ = strategist2Fills[msg.sender][quote]; should be moved after the first if.

Tools Used

VIM

Recommended Mitigation Steps

change the logic to:

    function strategistBootyClaim(address asset, address quote)
        external
        onlyApprovedStrategist(msg.sender)
    {
        uint256 fillCountA = strategist2Fills[msg.sender][asset];
        if (fillCountA > 0) {
            uint256 booty = (
                fillCountA.mul(IERC20(asset).balanceOf(address(this)))
            ).div(totalFillsPerAsset[asset]);
            IERC20(asset).transfer(msg.sender, booty);
            emit LogStrategistRewardClaim(
                msg.sender,
                asset,
                booty,
                block.timestamp
            );
            totalFillsPerAsset[asset] -= fillCountA;
            strategist2Fills[msg.sender][asset] -= fillCountA;
        }

        uint256 fillCountQ = strategist2Fills[msg.sender][quote];
        if (fillCountQ > 0) {
            uint256 booty = (
                fillCountQ.mul(IERC20(quote).balanceOf(address(this)))
            ).div(totalFillsPerAsset[quote]);
            IERC20(quote).transfer(msg.sender, booty);
            emit LogStrategistRewardClaim(
                msg.sender,
                quote,
                booty,
                block.timestamp
            );
            totalFillsPerAsset[quote] -= fillCountQ;
            strategist2Fills[msg.sender][quote] -= fillCountQ;
        }
    }

[KenzoAgada]

Note that in mitigation, the lines of:

            totalFillsPerAsset[asset] -= fillCountA;
            strategist2Fills[msg.sender][asset] -= fillCountA;

Can be changed to use SafeMath to consistently prevent underflows.
Same for quote token.

[HickupHH3]

Duplicate of #157

[HickupHH3]

Centralisation risk issue: #344