0xTheC0der - Insufficient validation of auction execution price adapter config data

[sherlock-admin2]

0xTheC0der

medium

Insufficient validation of auction execution price adapter config data

Summary

Mismatching auction execution price adapter config data is successfully validated, leading to unintended price curves and subsequent failure of the bidding process.

Vulnerability Detail

When starting a new rebalance auction via AuctionRebalanceModuleV1.startRebalance(...), all the provided auction execution parameters are validated using AuctionRebalanceModuleV1._validateAuctionExecutionPriceParams(...). This method retrieves a specified price adapter by name and subsequently uses its IAuctionPriceAdapterV1.isPriceAdapterConfigDataValid(...) method to validate the provided price config data.

However, there are many cases where IAuctionPriceAdapterV1.isPriceAdapterConfigDataValid(...) returns true even when provided with config data from a different adapter, since this method does not perform a length check of the provided data bytes nor does the data contain any sort price adapter identifier. Also the AuctionRebalanceModuleV1._validateAuctionExecutionPriceParams(...) method cannot check if the provided config data matches the price adapter.

For example:

• ConstantPriceAdapter.isPriceAdapterConfigDataValid(...) returns true when provided with config data for any other price adapter due to missing length check of the supplied config bytes.
• BoundedStepwiseExponentialPriceAdapter.isPriceAdapterConfigDataValid(...) has the exact same underlying areParamsValid(...) check as BoundedStepwiseLogarithmicPriceAdapter.isPriceAdapterConfigDataValid(...), therefore exponential/logarithmic config data can be mixed up among these two price adapters while the validation will always return true since the config bytes do not contain any sort of identifier.

Impact

The explicit price adapter config data validation is insufficient as soon as multiple price adapters come into play and therefore fails to prevent misconfiguration before pricing issues arise during the subsequent bidding process. Such a misconfiguration going unnoticed can lead to unintended price curves that therefore incur losses for the bidders or simply disincentivize bidders from helping to rebalance.
Furthermore, it's worth to mention that even the original test cases contain such an unnoticed misconfiguration (see comments in PoC code) which emphasizes the severity and likelihood of this issue.

Code Snippet

The following PoC modifies existing test cases to demonstrate the aforementioned example issues. Just apply the diff below and run the test with npx hardhat test test/protocol/modules/v1/auctionRebalanceModuleV1.spec.ts.

diff --git a/test/protocol/modules/v1/auctionRebalanceModuleV1.spec.ts b/test/protocol/modules/v1/auctionRebalanceModuleV1.spec.ts
index fc35069..74388eb 100644
--- a/test/protocol/modules/v1/auctionRebalanceModuleV1.spec.ts
+++ b/test/protocol/modules/v1/auctionRebalanceModuleV1.spec.ts
@@ -2788,7 +2788,7 @@ describe("AuctionRebalanceModuleV1", () => {
         });
       });
 
-      describe("when the bid is priced using the BoundedStepwiseLinearPriceAdapter", async () => {
+      describe.only("when the bid is priced using the BoundedStepwiseLinearPriceAdapter", async () => {
         let oldComponentsAuctionParams: AuctionExecutionParams[];
 
         beforeEach(async () => {
@@ -2834,6 +2834,7 @@ describe("AuctionRebalanceModuleV1", () => {
             {
               targetUnit: ether(4),
               priceAdapterName: AdapterNames.CONSTANT_PRICE_ADAPTER,
+              // @audit mistake in original test case goes unnoticed since const price adapter accepts other config data too
               priceAdapterConfigData: wethLinearCurveParams
             }
           ];
@@ -2972,7 +2973,7 @@ describe("AuctionRebalanceModuleV1", () => {
         });
       });
 
-      describe("when the bid is priced using the BoundedStepwiseExponentialPriceAdapter", async () => {
+      describe.only("when the bid is priced using the BoundedStepwiseExponentialPriceAdapter", async () => {
         let oldComponentsAuctionParams: AuctionExecutionParams[];
 
         beforeEach(async () => {
@@ -3010,7 +3011,8 @@ describe("AuctionRebalanceModuleV1", () => {
           oldComponentsAuctionParams = [
             {
               targetUnit: ether(9100),
-              priceAdapterName: AdapterNames.BOUNDED_STEPWISE_EXPONENTIAL_PRICE_ADAPTER,
+              // @audit can use logarithmic price adapter with exponential config data without "Price adapter config data invalid" error
+              priceAdapterName: AdapterNames.BOUNDED_STEPWISE_LOGARITHMIC_PRICE_ADAPTER,
               priceAdapterConfigData: daiExponentialCurveParams
             },
             {
@@ -3159,7 +3161,7 @@ describe("AuctionRebalanceModuleV1", () => {
         });
       });
 
-      describe("when the bid is priced using the BoundedStepwiseLogarithmicPriceAdapter", async () => {
+      describe.only("when the bid is priced using the BoundedStepwiseLogarithmicPriceAdapter", async () => {
         let oldComponentsAuctionParams: AuctionExecutionParams[];
 
         beforeEach(async () => {
@@ -3197,7 +3199,8 @@ describe("AuctionRebalanceModuleV1", () => {
           oldComponentsAuctionParams = [
             {
               targetUnit: ether(9100),
-              priceAdapterName: AdapterNames.BOUNDED_STEPWISE_LOGARITHMIC_PRICE_ADAPTER,
+              // @audit can use exponential price adapter with logarithmic config data without "Price adapter config data invalid" error
+              priceAdapterName: AdapterNames.BOUNDED_STEPWISE_EXPONENTIAL_PRICE_ADAPTER,
               priceAdapterConfigData: daiLogarithmicCurveParams
             },
             {

Log:

  AuctionRebalanceModuleV1
    when module is initalized
      #bid
        when the bid is priced using the BoundedStepwiseLinearPriceAdapter
          when the bid is placed on a component sell auction
            √ updates position units and transfers tokens correctly on a component sell auction with BoundedStepwiseLinearPriceAdapter (102ms)
            √ emits the correct BidExecuted event (80ms)
          when the bid is placed on a component buy auction
            √ updates position units and transfers tokens correctly on a component buy auction with BoundedStepwiseLinearPriceAdapter (87ms)
            √ emits the correct BidExecuted event (64ms)
        when the bid is priced using the BoundedStepwiseExponentialPriceAdapter
          when the bid is placed on a component sell auction
            √ updates position units and transfers tokens correctly on a sell auction with BoundedStepwiseExponentialPriceAdapter (88ms)
            1) emits the correct BidExecuted event
          when the bid is placed on a component buy auction
            √ updates position units and transfers tokens correctly on a buy auction with BoundedStepwiseExponentialPriceAdapter (90ms)
            √ emits the correct BidExecuted event (65ms)
        when the bid is priced using the BoundedStepwiseLogarithmicPriceAdapter
          when the bid is placed on a component sell auction
            √ updates position units and transfers tokens correctly on a sell auction with BoundedStepwiseLogarithmicPriceAdapter (89ms)
            2) emits the correct BidExecuted event
          when the bid is placed on a component buy auction
            √ updates position units and transfers tokens correctly on a buy auction with BoundedStepwiseLogarithmicPriceAdapter (87ms)
            √ emits the correct BidExecuted event (64ms)


  10 passing (5s)
  2 failing

The test cases show that the misconfiguration goes completely unnoticed in AuctionRebalanceModuleV1.startRebalance(...), even most of the bidding process runs successfully.

Tool used

Manual Review

Recommendation

Check the length of the provided config data bytes in the respective IAuctionPriceAdapterV1.isPriceAdapterConfigDataValid(...) methods and add a price adapter identifier (e.g. first 4 bytes of namehash) to the config data in order to prevent successful validation of mismatching data.

Alternative: In case the current validation behaviour is intended and the SetToken manager is trusted to always provide correct config data, the validation method can be removed from the contract to save gas.