c4-bot-7 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L47-L62 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L37-L44

Vulnerability details

Impact

The PriceAggregator.setPriceFeed function can only be called to change a particula priceFeed once the priceFeedModificationCooldownExpiration is reached. This is verified by the following conditional check.

    if ( block.timestamp < priceFeedModificationCooldownExpiration )
        return;

The Natspec comments describe this check is performed to allow time to evaluate the performance of the recently updated PriceFeed before further updates are made. Hence a new priceFeed update can only be performed after the period of priceFeedModificationCooldown since the last update.

But the issue is priceFeedModificationCooldownExpiration value is not updated in the PriceAggregator.setInitialFeeds function. As a result immediately after the setInitialFeeds is called to set the priceFeeds the setPriceFeed function can be called to change a particular priceFeed thus not alowing enough time to evaluate the performance of the recently updated priceFeeds.

Hence this breaks the intended behavior of the priceFeeds update mechanism since no cool down period is enforced after the PriceAggregator.setInitialFeed function is called to setup the initial priceFeeds.

Proof of Concept

    function setPriceFeed( uint256 priceFeedNum, IPriceFeed newPriceFeed ) public onlyOwner
        {
        // If the required cooldown is not met, simply return without reverting so that the original proposal can be finalized and new setPriceFeed proposals can be made.
        if ( block.timestamp < priceFeedModificationCooldownExpiration )
            return;

        if ( priceFeedNum == 1 )
            priceFeed1 = newPriceFeed;
        else if ( priceFeedNum == 2 )
            priceFeed2 = newPriceFeed;
        else if ( priceFeedNum == 3 )
            priceFeed3 = newPriceFeed;

        priceFeedModificationCooldownExpiration = block.timestamp + priceFeedModificationCooldown;
        emit PriceFeedSet(priceFeedNum, newPriceFeed);
        }

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L47-L62

    function setInitialFeeds( IPriceFeed _priceFeed1, IPriceFeed _priceFeed2, IPriceFeed _priceFeed3 ) public onlyOwner
        {
        require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );

        priceFeed1 = _priceFeed1;
        priceFeed2 = _priceFeed2;
        priceFeed3 = _priceFeed3;
        }

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L37-L44

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to enforce the cool down period setup in the PriceAggregator.setInitialFeed function once the priceFeeds are initially setup. This will ensure the priceFeedModificationCooldownExpiration is properly setup for verification when the PriceAggregator.setPriceFeed function is called. The PriceAggregator.setInitialFeed function can be updated as shown below to implement the above recommendation.

function setInitialFeeds( IPriceFeed _priceFeed1, IPriceFeed _priceFeed2, IPriceFeed _priceFeed3 ) public onlyOwner
    {
    require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );
    priceFeed1 = _priceFeed1;
    priceFeed2 = _priceFeed2;
    priceFeed3 = _priceFeed3;

    priceFeedModificationCooldownExpiration = block.timestamp + priceFeedModificationCooldown;       
    }

Assessed type

Invalid Validation

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #970

c4-judge commented 6 months ago

Picodes marked the issue as satisfactory