Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M-07 MitigationConfirmed #7

Open
c4-bot-2 opened this issue Sep 14, 2024 · 1 comment
Open

M-07 MitigationConfirmed #7

c4-bot-2 opened this issue Sep 14, 2024 · 1 comment
Labels
mitigation-confirmed MR-M-07 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-2
Copy link
Contributor

Lines of code

Vulnerability details

See:

Finding Mitigation
M-07: The traceEnd in BackingManager isn't updating correctly Pull Request

Navigating to M-07 from the previous contest we can see that is a vulnerability in the BackingManager contract where the tradeEnd value, used to prevent multiple auctions from occurring in the same block, can inadvertently block the next Dutch auction from starting for an extended period. This occurs because the tradeEnd is set to the auction's end time, which can be up to a week later. If the basket status changes from IFFY to SOUND during this period, a new auction cannot be created until the tradeEnd expires, even if conditions are favorable. This delay poses an issue for timely rebalancing and affects the avaialbility of the protocol. The recommended mitigation was to update the tradeEnd to the current block timestamp in the settleTrade function, allowing new auctions to be created as soon as conditions permit, whereas this was not directly applied in the fix, the impactful window of this issue to Reserve has been sufficiently fixed in the pull request used to solve this, considering Reserve claimed that the dutch auctions were not really important to them post PJQA as can be seen here, so what they really care to fix is the unblocking of rebalancing after an auction ends early and now we pick the minimum between the current timestamp and the tradeEnd when settling the trades ensuring the availability is maintained.

    function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
        delete tokensOut[sell];
+       tradeEnd[trade.KIND()] = uint48(Math.min(tradeEnd[trade.KIND()], block.timestamp));
        trade = super.settleTrade(sell); // nonReentrant

        // if the settler is the trade contract itself, try chaining with another rebalance()
        if (_msgSender() == address(trade)) {
            // solhint-disable-next-line no-empty-blocks
            try this.rebalance(trade.KIND()) {} catch (bytes memory errData) {
                // prevent MEV searchers from providing less gas on purpose by reverting if OOG
                // untested:
                //     OOG pattern tested in other contracts, cost to test here is high
                // see: docs/solidity-style.md#Catching-Empty-Data
                if (errData.length == 0) revert(); // solhint-disable-line reason-string
            }
        }
    }
@c4-judge
Copy link

thereksfour marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mitigation-confirmed MR-M-07 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants