Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

ss3434 - _etherPrice() in Kept.sol missing checking sequencer status on L2 #25

Closed
sherlock-admin opened this issue Aug 15, 2023 · 1 comment
Labels
Non-Reward This issue will not receive a payout

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Aug 15, 2023

ss3434

medium

_etherPrice() in Kept.sol missing checking sequencer status on L2

Summary

Using Chainlink in L2 chains such as Arbitrum requires to check if the sequencer is down to avoid prices from looking like they are fresh although they are not.

Vulnerability Detail

modifier keep uses _etherPrice() for calculation keeperFee.

  modifier keep(UFixed18 multiplier, uint256 buffer, bytes memory data) {
        uint256 startGas = gasleft();

        _;

        uint256 gasUsed = startGas - gasleft();
        UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
            .mul(multiplier)
            .add(UFixed18Lib.from(buffer))
            .mul(_etherPrice())
            .mul(UFixed18.wrap(block.basefee));

        _raiseKeeperFee(keeperFee, data);

        keeperToken().push(msg.sender, keeperFee);

        emit KeeperCall(msg.sender, gasUsed, multiplier, buffer, keeperFee);
    }

The _etherPrice() function is a function in the Kept.sol contract that fetches the latest price data from a Chainlink price feed.

 function _etherPrice() private view returns (UFixed18) {
        (, int256 answer, , ,) = ethTokenOracleFeed().latestRoundData();
        return UFixed18Lib.from(Fixed18Lib.ratio(answer, 1e8)); // chainlink eth-usd feed uses 8 decimals
    }

Also it's missing checking sequencer status on L2.
From Chainlink documentation:
Optimistic rollup protocols have a sequencer that executes and rolls up the L2 transactions by batching multiple transactions into a single transaction.
If a sequencer becomes unavailable, it is impossible to access read/write APIs that consumers are using and applications on the L2 network will be down for most users.
This means that if the project does not check if the sequencer is down, it can return stale results.

Impact

Posible wrong calculation keeperFee in modifier keep

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/attribute/Kept.sol#L40C3-L57C6
https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/attribute/Kept.sol#L61-L64

Tool used

Manual Review

Recommendation

Check L2 sequencer status and data staleness by time interval.
From Chainlink documentation:

Duplicate of #146

@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Aug 18, 2023
@sherlock-admin
Copy link
Contributor Author

2 comment(s) were left on this issue during the judging contest.

141345 commented:

d

n33k commented:

unhandled stale price returned from latestRoundData()

@sherlock-admin sherlock-admin changed the title Stale Chambray Snake - _etherPrice() in Kept.sol missing checking sequencer status on L2 ss3434 - _etherPrice() in Kept.sol missing checking sequencer status on L2 Aug 23, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

1 participant