Looping over unbounded array of yangs may lead to DoS

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/seer.cairo#L202-L230 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/sentinel.cairo#L122-L133

Vulnerability details

Impact

In seer.cairo file, function update_prices_internal loops over an array of all yangs:

File: seer.cairo

 // loop through all yangs
            // for each yang, loop through all oracles until a
            // valid price update is fetched, in which case, call shrine.advance()
            // the expectation is that the primary oracle will provide a
            // valid price in most cases, but if not, we can fallback to other oracles
            let mut yangs: Span<ContractAddress> = sentinel.get_yang_addresses();
            loop {
                match yangs.pop_front() {

However, the number of elements in yangs is nowhere limited. When this array contains a lot of elements - looping will eventually lead to out of gas error.

Proof of Concept

File: seer.cairo

 let mut yangs: Span<ContractAddress> = sentinel.get_yang_addresses();
            loop {
                match yangs.pop_front() {

Function sentinel.get_yang_addresses() gets the list of all the yangs in the protocol. It's implemented as below:

File: sentinel.cairo

        fn get_yang_addresses(self: @ContractState) -> Span<ContractAddress> {
            let mut idx: u64 = LOOP_START;
            let loop_end: u64 = self.yang_addresses_count.read() + LOOP_START;
            let mut addresses: Array<ContractAddress> = ArrayTrait::new();
            loop {
                if idx == loop_end {
                    break addresses.span();
                }
                addresses.append(self.yang_addresses.read(idx));
                idx += 1;
            }
        }

As demonstrated above, it gets all the yangs in the protocol. When protocols has a lot of yangs, the list returned by sentinel.get_yang_addresses() will be extremely big. Moreover, function update_prices_internal from seer.cairo will iterate over that list. This constitues an issue, since, looping over an unbounded list may result in DoS (revert with out of gas error).

Function update_prices_internal is called whenever we want to either execute a task (execute_task) or update prices (update_prices). When it will revert with out of gas error, it won't be possible to properly use the protocol (since execute_task and update_prices) will revert too.

It's mandatory to make sure that we are not looping over too large array of yangs.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement additional check which won't allow to add so many yangs, that update_prices_internal won't be able to loop over them.

Assessed type

DoS

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

The Warden specifies how an unbounded iteration of the Yangs (collaterals) in the system may result in an out-of-gas error.

This is an invalid submission as the system would have to be configured with an abnormally large number of collaterals by an administrator of the Opus team.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid