[WP-H3] `money-market-contracts/oracle#feed_prices()` delayed transaction may disrupt price feeds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L106-L113

Vulnerability details

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L106-L113

The implementation only takes two attributes: asset and price. And the last_updated_time of the record will always be set to the current block.time.

This makes it possible for the price feeds to be disrupted when the network is congested, or the endpoint is down for a while, or the feeder bot handled the message queue inappropriately, as a result, the transactions with stale prices get accepted as fresh prices.

Since the price feeds are essential to the protocol, that can result in users' positions being liquidated wrongfully and case fund loss to users.

PoC

Given:

• feeder i connected to an endpoint currently experiencing degraded performance;
• ETH price is $10,000;
• The max_ltv ratio of ETH is 60%.

1. Alice borrowed 5,000 USDC with 1 ETH as collateral;
2. ETH price dropped to $9,000, to avoid liquidation, Alice repaid 1,000 USD;
3. The price of ETH dropped to $8,000; feeder tries to updateMainFeedData() with the latest price: $8,000, however, since the network is congested, the transactions were not get packed timely;
4. ETH price rebound to $10,000; Alice borrowed another 1,000 USDC;
5. The txs send by feeder at step 3 finally got packed, the protocol now believes the price of ETH has suddenly dropped to $8,000, as a result, Alice's position got liquidated.

Recommendation

Change to:

pub fn feed_prices(
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    prices: Vec<(String, Decimal256, u64)>,
) -> Result<Response, ContractError> {
    let mut attributes = vec![attr("action", "feed_prices")];
    let sender_raw = deps.api.addr_canonicalize(info.sender.as_str())?;
    for price in prices {
        let asset: String = price.0;
        let mut updated_time: u64 = price.2;
        let price: Decimal256 = price.1;

        // Check feeder permission
        let feeder = read_feeder(deps.storage, &asset)?;
        if feeder != sender_raw {
            return Err(ContractError::Unauthorized {});
        }

        let config: Config = read_config(deps.storage)?;
        if env.block.time.seconds() > updated_time {
            // reject stale price
            if env.block.time.seconds() - updated_time > config.valid_period {
                return Err(ContractError::InvalidInputs {});
            }
        } else {
            // reject future timestamp, graceFuturePeriod can be set to 3, which means < 3s is allowed
            if updated_time - env.block.time.seconds() > config.grace_future_period {
                return Err(ContractError::InvalidInputs {});
            }
            updated_time = env.block.time.seconds();
        }

        attributes.push(attr("asset", asset.to_string()));
        attributes.push(attr("price", price.to_string()));

        store_price(
            deps.storage,
            &asset,
            &PriceInfo {
                last_updated_time: updated_time,
                price,
            },
        )?;
    }

    Ok(Response::new().add_attributes(attributes))
}

[bitn8]

We currently have a mean shorting function that pulls multiple price feeds so that if one is stale it gets rejected.

[GalloDaSballo]

Seems like the warden has shown a specific scenario, contingent on external conditions.

However, from the code, there seems to be no "mean shorting function", at least in the code in scope

[albertchon]

Agreed with @GalloDaSballo , oracle staleness is still an issue in this version of the code.