Salty.IO - miaowu's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreSaltyFeed.sol#L32

Vulnerability details

Impact

In the src/price_feed/PriceAggregator.sol file, in the function _aggregatePrices: https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L108-L146

if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
            (priceA, priceB) = (price1, price2);
        else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
            (priceA, priceB) = (price1, price3);
        else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
            (priceA, priceB) = (price2, price3);

        uint256 averagePrice = ( priceA + priceB ) / 2;

averagePrice calculates the price by returning the average of the two with the smallest price difference among the three oracles (CoreChainlinkFeed, CoreSaltyFeed, CoreUniswapFeed).

However, in the oracle src/price_feed/CoreSaltyFeed.sol, the getPriceBTC function and getPriceETH use instantaneous prices, so attackers can manipulate the price of this oracle through flash loans and profit from it:

1. When Chainlink melts down (Chainlink has blown down several times in history, so this is a possibility), only the average of CoreSaltyFeed and CoreUniswapFeed prices can be taken
2. Manipulate the price of CoreSaltyFeed through flash loans
3. Manipulate averagePrice and profit from the token price difference

//CoreSaltyFeed.sol
function getPriceBTC() external view returns (uint256)
        {
        (uint256 reservesWBTC, uint256 reservesUSDS) = pools.getPoolReserves(wbtc, usds);

        if ( ( reservesWBTC < PoolUtils.DUST ) || ( reservesUSDS < PoolUtils.DUST ) )
            return 0;

        // reservesWBTC has 8 decimals, keep the 18 decimals of reservesUSDS
        return ( reservesUSDS * 10**8 ) / reservesWBTC;
        }

//Utils.sol
function corePrices(IPools pools, IExchangeConfig exchangeConfig, IPriceAggregator priceAggregator) public view returns (uint256 btcPrice, uint256 ethPrice, uint256 saltPrice, uint256 usdsPrice)
{
btcPrice =  priceAggregator.getPriceBTC();
...
}

Tools Used

Manual review

Recommended Mitigation Steps

For market price oracles such as Uniswap or OasisDEX, due to the possibility of lightning attacks, you cannot use the current median market price as a price feed under any circumstances. An attacker can easily and significantly change the median market price with just one transaction, causing the oracle to malfunction. The best solution to this is to calculate the weighted average of the last batch of X blocks via Time Weighted Average Price (TWAP) or Volume Weighted Average Price (VWAP).

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L103

Vulnerability details

Impact

There is a problem with the proposal naming rules, and there is a possibility of duplication of proposal names at different stages. When you want to add the suffix "_confirm" to a confirmed proposal, it will be checked to see if a proposal with the same name already exists. If there is a proposal with the same name, it cannot be created. This allows an attacker to create a proposal with ballotName + "_confirm" so that the proposal can never advance to the next stage.

Tools Used

Manual review

Recommended Mitigation Steps

When creating and naming files, it is forbidden to use names with the suffix "_confirm". In this way, there is no possibility of duplication of proposal titles at different stages.

// Make sure that a proposal of the same name is not already open for the ballot
        require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
        require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

Assessed type

DoS