Spark review fixes

[hexonaut]

Aggor Review 2. I'll respond to each of the points from my previous review.

Previous Review for Reference:

1. Why is —via-ir being set? I think there are some trade offs with enabling this. Is it for the stack too deep? It would be better to just eliminate the stack too deep errors.
2. Test coverage needs to be improved. I see 5 tests, and I cannot run forge coverage to see due to the same —via-ir issue.
3. Decimals should be enforced to be 8. It seems to set from whatever Chainlink oracle is.
4. Let's drop the pegged asset mode. We are no longer going to be using this as after consultation with the Risk team we are using the primary market for staked eth which doesn't require a market price. Less code is preferable for security reasons.
5. Re: ChainSecurity 8.4 point, upgradability is indeed an attack vector. We are trying to protect against fully malicious circumstances, so we need to do a try catch on the Chainlink price feed.
6. Re: ChainSecurity 5.1 and 5.2 checks added to the constructor. I don’t agree with the saving gas reason to not include. It’s always better to eliminate footguns if possible.
7. `_inAgreementDistance` can be optimized. Store `agreementDistance` as a WAD instead of BPS to drop the extra multiplications/divisions. The `a == b` can be dropped - it will likely never happen in practise and the case is covered by the general version.

1. Looks good now.
2. Running forge coverage the coverage is still not 100% on Aggor.sol.
3. Fixed.
4. Fixed.
5. Can a realistic failure mode test be added for this? Chainlink price feed will be 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419. The interface is not upgradable on that contract, so how would it return anything other than what is expected or revert?
6. Fixed.
7. Further optimization. agreementDistance semantics can be changed to have 1 WAD represent identical values to remove the subtraction in the calculation. IE agreementDistance = 0.95e18 implies a 5% deviation of price instead of setting the value to 0.05e18 with the current setup. This will remove a subtraction and this is important because it's a hot path. Also can remove a division/stack var. I've rewritten the code below.

uint diff = a > b
    ? 1e18 - uint(b) * 1e18 / uint(a)
    : 1e18 - uint(a) * 1e18 / uint(b);

// And return whether %-difference inside acceptable agreement distance.
return diff <= agreementDistance;

becomes

if (a > b) {
    return uint(b) * 1e18 >= agreementDistance * uint(a);
} else {
    return uint(a) * 1e18 >= agreementDistance * uint(b);
}

Also, I ran forge test and got some failures:

[FAIL. Reason: revert: NotImplemented] testFuzz_read_ChronicleNotOk_ChainlinkNotOk_TwapNotOk() (gas: 281)
[FAIL. Reason: revert: NotImplemented; counterexample: calldata=0x89c7d318000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 args=[0, 0, 0]] testFuzz_read_ChronicleOk_ChainlinkOk_NotInAgreementDistance_TwapNotOk(uint128,uint256,uint256) (runs: 0, μ: 0, ~: 0)

[pmerkleplant]

Thanks for the review!

About 2) code coverage not 100%: Added functionality to include the _verifyTwapConfig() function, which is only used in the constructor, to also be included in the coverage. Forge does not normally include construction code in the coverage report.

However, note that the Uniswap TWAP fails if the price returned overflows a uint128. We were unable to construct a mocked Uniswap pool state with such a high price and also couldn't find any real pool to use in a fork test. Therefore, there are two test cases that revert with NotImplemented() - one for each case the twap is read.

Except of those 2 cases, the coverage is complete.

About 5) chainlink oracle not upgradeable: Yes, as this oracle instance does not use a proxy a try-catch would be sufficient, ie a call either succeeds with the expected return data or it reverts. However, note that for a deployment of aggor for a different asset pair the proxy defense mechanisms would need to be implemented again - which may be easy to forget.

About 7) agreementDistance semantic: Fixed, nice pattern!

[hexonaut]

5) I would like to proceed with the switch to try catch on the Chainlink price feed. There shouldn't be any more complexity than needed.

Coverage looks better now, but I'm still getting test failures.

[FAIL. Reason: revert: NotImplemented] testFuzz_read_ChronicleNotOk_ChainlinkNotOk_TwapNotOk() (gas: 324)
[FAIL. Reason: revert: NotImplemented; counterexample: calldata=0x89c7d318000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 args=[0, 0, 0]] testFuzz_read_ChronicleOk_ChainlinkOk_NotInAgreementDistance_TwapNotOk(uint128,uint256,uint256) (runs: 0, μ: 0, ~: 0)

[lucas-manuel]

Great PR and great tests!

Couple of notes:

1. I think we should use error messages in requires so that the tests can demonstrate with 100% certainty that the code is reverting for the reason demonstrated.
2. The fuzz tests here are great, there are some situations though where an additional "boundary" test can be much more effective. An easy example is testFuzz_setAgreementDistance_RevertsIf_AgreementDistanceMoreThan1WAD. That test fuzzes the entire state space, and only shows one side of the condition. With something like the below example, you can demonstrate the exact boundary for when a condition is met and not met and no fuzzing is needed:

    function test_setAgreementDistance_RevertsIf_AgreementDistanceMoreThan1WAD() public {
        vm.expectRevert();
        aggor.setAgreementDistance(1e18 + 1);

        aggor.setAgreementDistance(1e18);
    }

I think this approach would be great to apply to the IAggor.Status.path values changing depending on:

1. Agreement distance (show exact point where one path is taken vs another)
2. Chainlink value overflow
3. Stale timestamps (setting to age + ageThreshold boundary to show different paths)

Looks solid otherwise!