code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/1136e0cdc8579614a33832fe2a21785d60aac19b/protocol/contracts/tokenomics/InflationManager.sol#L110-L125

Vulnerability details

Impact

If there are too many keeperGauges + stakerVaults + ammGauges

checkpointAllGauges functions may loop too mamy times which cause gas to be explode and always reverted.

Proof of Concept

    function checkpointAllGauges() external override returns (bool) {
        uint256 length = _keeperGauges.length();
        for (uint256 i; i < length; i = i.uncheckedInc()) {
            IKeeperGauge(_keeperGauges.valueAt(i)).poolCheckpoint();
        }
        address[] memory stakerVaults = addressProvider.allStakerVaults();
        for (uint256 i; i < stakerVaults.length; i = i.uncheckedInc()) {
            IStakerVault(stakerVaults[i]).poolCheckpoint();
        }

        length = _ammGauges.length();
        for (uint256 i; i < length; i = i.uncheckedInc()) {
            IAmmGauge(_ammGauges.valueAt(i)).poolCheckpoint();
        }
        return true;
    }

If there are too many keeperGauges + stakerVaults + ammGauges, checkpointAllGauges functions may loop too mamy times which cause gas to be explode and always reverted.

This case happended to Pancakeswap as they have over hundreds farms. They cannot massUpdatePool and individual pools are required to be update independently.

Tools Used

Manual code scanning

Recommended Mitigation Steps

Split checkpointAllGauges into multiple checkpoint with start and end looping index

chase-manning commented 2 years ago

The amount of gauges will be kept to a minimum

GalloDaSballo commented 2 years ago

I believe the finding to have validity, however:

In lack of any meaningful POC with amount of gauges (I believe it would take at least over 10k entries for OOG to be considered)
Because poolCheckpoint can still be called on a one to one basis (they are permissioneless operations)

I will downgrade to QA

code-423n4 / 2022-05-backd-findings