THE `PriceAggregator.setInitialFeeds` FUNCTION CAN BE CALLED MULTIPLE TIMES BY THE `owner` EVEN THOUGH IT SHOULD BE CALLED ONLY ONCE AS PER THE FUNCTION DESIGN

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L37-L44 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L50-L51 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L122-L123 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L151-L158

Vulnerability details

Impact

The PriceAggregator.setInitialFeeds function is used to set the initial price feeds and only be called by the owner. And this function can only be called once by the owner as it is depicted in the following conditional check.

    require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );

And if the owner needs to change the priceFeeds there after he will have to call the PriceAggregator.setPriceFeed function which has a price modification cool down period implemented. This is why the setInitialFeeds is implemented as a function that can be called only once.

And the way the priceAggregator._aggregatePrices function works, even two price feeds will be enough for the price aggregator to work properly since the following condition will be bypassed with two valid price feeds.

    if ( numNonZero < 2 )
        return 0;

Hence the owner can call PriceAggregator.setInitialFeeds function multiple times to change the priceFeed2 and priceFeed3 as he needs without complementing the price modification cool down period.

Let's consider the following scenario:

1. The owner calls the PriceAggregator.setInitialFeeds function with priceFeeds1 == address(0) and provides valid price feed addresses to _priceFeed2 and _priceFeed3.
2. As a result the priceFeed1 will remain == address(0).
3. Owner can now bypass the require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" ) check to call the PriceAggregator.setInitialFeeds function to change the priceFeed2 and priceFeed3 as he wishes without complementing the price modification cool down period
4. Even with the priceFeed1 being address(0) the price aggregator will work fine since call to the priceFeed1 (in the PriceAggregator._getPriceBTC and priceAggregator._getPriceETH functions) is performed in a try-catch block and as a result the call to address(0) will not revert the transaction.
5. And further two priceFeeds are enough for the priceAggregator contract to work properly.

Hence if the owner requires he can decide to keep on changing the priceFeed2 and priceFeed3 as he needs with out complementing the price modification cool down period. This will make the PriceAggregator.setPriceFeed function less useful since its price modification cool down period implementation can be bypassed by the owner easily, to change the priceFeeds as the owner wishes. And furthermore this should not be considered as a admin mistake since the PriceAggregator.setInitialFeeds function is designed in such a way that even the owner is not allowed to call the function more than once. This vulnerability clearly shows how the owner can call the PriceAggregator.setInitialFeeds function to change the priceFeeds2 and priceFeeds3 multiple times contrary to what the function is designed to do.

Note : Even though the L1 or the automated report states User facing functions should have address(0) checks it does not mention this exact vulnerability and the impact of it.

Proof of Concept

    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

        if ( block.timestamp < priceFeedModificationCooldownExpiration )
            return;

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

        if ( numNonZero < 2 )
            return 0;

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

        try priceFeed.getPriceBTC() returns (uint256 _price)
            {
            price = _price;
            }
        catch (bytes memory)
            {
            // price remains 0
            }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to implement an address(0) check for the _priceFeed1 address in the PriceAggregator.setInitialFeeds function such that the transaction will revert if the address(0) is passed in as the _priceFeed1. This will ensure that the _priceFeed1 is set to a non-zero value and the PriceAggregator.setInitialFeeds can only be called once as a result.

require(address(_priceFeed1) != address(0), "_priceFeed1 can not be address(0)");

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

This is not an issue as the contract is deployed with non zero price feeds and checked at the time of deployment.

[Picodes]

This report shows how the initial deployer could misconfigure the system to exploit it later, either by using an address(0), either as there is initially no cooldown. As it falls within centralization risk and misconfiguration issues I'll downgrade to QA.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b