The `GnosisTrade::init` function will always revert due to difference in initial `TradeStatus` requirement by `GnosisTrade::stateTransition` modifier and TradeStatus value set in the constructor at deployment

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/GnosisTrade.sol#L66-L90

Vulnerability details

Impact

Summary

The GnosisTrade::init function is designed to set the GnosisTrade::status variable to TradeStatus.OPEN so that trading positions can be opened on the GnosisTrade contract. However, the GnosisTrade::init function has a modifier which ensures that status == TradeStatus.NOT_STARTED before the function executes whereas, GnosisTrade::status was set to TradeStatus.CLOSED at deployment causing the function to revert.

Vulnerability Details

The vulnerability lies in the fact that

1. the constructor function sets status as TradeStatus.CLOSED as indicated below

   
   constructor() {
   @>      status = TradeStatus.CLOSED;
   }

2. whereas, the `stateTransition` modifier requires `status == TradeStatus.NOT_STARTED` as shown below
```javascript
function init(
        IBroker broker_,
        address origin_,
        IGnosis gnosis_,
        uint48 batchAuctionLength,
        TradeRequest calldata req
@>  ) external stateTransition(TradeStatus.NOT_STARTED, TradeStatus.OPEN) {

Impact

Since the GnosisTrade::stateTransition modifier causes the GnosisTrade::init function to always revert due to invalid trade status, no trade can be initiated on the GnosisTrade contract thereby affecting the protocol's functionality.

Proof of Concept

1. Initialize a foundry project in the codebase using forge init

2. Go to the /src directory created by foundry, delete the Counter.t.sol file and create a new file named ReserveMain.t.sol with the following input

   // SPDX-License-Identifier: UNLICENSED
   pragma solidity ^0.8.0;
   
   // import {Test, console} from "forge-std/Test.sol";
   import "forge-std/Test.sol";
   import { RevenueTraderP1, IMain, IERC20 } from "../contracts/p1/RevenueTrader.sol";
   import { MainP1 } from "../contracts/p1/Main.sol";
   import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
   import { GnosisTrade } from "../contracts/plugins/trading/GnosisTrade.sol";
   import { DutchTrade } from "../contracts/plugins/trading/DutchTrade.sol";
   import { BrokerP1 } from "../contracts/p1/Broker.sol";
   import { IBroker, TradeRequest } from "../contracts/interfaces/IBroker.sol";
   import { IAsset } from "../contracts/interfaces/IAsset.sol";
   import { IGnosis } from "../contracts/interfaces/IGnosis.sol";
   import { AssetRegistryP1 } from "../contracts/p1/AssetRegistry.sol";
   import { Governance } from "../contracts/plugins/governance/Governance.sol";
   
   import { IStRSRVotes } from "../contracts/interfaces/IStRSRVotes.sol";
   import { TimelockController } from "@openzeppelin/contracts/governance/TimelockController.sol";
   import { RoleRegistry } from "../contracts/registry/RoleRegistry.sol";
   import { ITrading } from "../contracts/interfaces/ITrading.sol";
   import { TradePrices } from "../contracts/interfaces/IBroker.sol";
   
   contract ReserveMainTest is Test {
       RevenueTraderP1 public rTrader;
       // IERC20 public tokenToBuy;
       ERC20 public tokenOne;
       ERC20 public tokenTwo;
       ERC20 public rsr_Token;
       IMain public main;
       GnosisTrade public gTrade;
       DutchTrade public dTrade;
       BrokerP1 public broker;
       AssetRegistryP1 public assetRegistry;
       Governance public governor;
       RoleRegistry public roleRegistry;
   
       address origin = makeAddr("origin");
   
       function setUp() public {
           rTrader = new RevenueTraderP1();
           tokenOne = new ERC20("SpoToken", "SPT");
           tokenTwo = new ERC20("MyToken", "MT");
           rsr_Token = new ERC20("RsrToken", "RST");
           main = IMain(new MainP1());
           broker = new BrokerP1();
   
           // parameters need to deploy the TimelockController contract
           uint256 minDelay = 20; 
           address[] memory proposers = new address[](2);
           address proposer1 = makeAddr("proposer1");
           address proposer2 = makeAddr("proposer2");
           proposers[0] = proposer1;
           proposers[1] = proposer2;
   
           address[] memory executors = new address[](2);
           address executor1 = makeAddr("executor1");
           address executor2 = makeAddr("executor2");
           executors[0] = executor1;
           executors[1] = executor2;
   
           address admin = makeAddr("ADMIN");
   
           // parameters need to deploy the Governance contract
           IStRSRVotes token_ = IStRSRVotes(address(rsr_Token));
           TimelockController timelock_ = new TimelockController(minDelay, proposers, executors, admin);
   
           uint256 votingDelay_ = 86400; // {s}
           uint256 votingPeriod_ = 300; // {s}
           uint256 proposalThresholdAsMicroPercent_ = 1e4; // e.g. 1e4 for 0.01%
           uint256 quorumPercent = 4; // e.g 4 for 4%
   
           governor = new Governance(token_, timelock_, votingDelay_, votingPeriod_, proposalThresholdAsMicroPercent_, quorumPercent);
   
           // give governor all admin roles
           vm.prank(address(governor));
           roleRegistry = new RoleRegistry();
       }
   }

3. Set up the test environment using the code below which deploys the necessary contracts and calls the GnosisTrade::init function after deployment

PoC

Place the following code into `ReserveMain.t.sol`. ```javascript function testGnosisTradeFailsToInitialize() public { TradeRequest memory req = TradeRequest({ sell: IAsset(address(tokenOne)), buy: IAsset(address(tokenTwo)), sellAmount: 10e18, minBuyAmount: 5e18 }); uint48 batchAuctionLength = 2; gTrade = new GnosisTrade(); IGnosis gnosis_ = IGnosis(address(gTrade)); // Test is expected to fail since TradeStatus is set as CLOSED in the constructor function while the modifier checks if TradeStatus is NOT_STARTED vm.expectRevert(); gTrade.init(broker, origin, gnosis_, batchAuctionLength, req); } ``` Now run `forge test --match-test testGnosisTradeFailsToInitialize -vvvv` Output: ```javascript . . . ├─ [959] GnosisTrade::init(BrokerP1: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], origin: [0xF0904054478098Bb6F21E30735A703Acc2823b5B], GnosisTrade: [0x03A6a84cD762D9707A21605b548aaaB891562aAb], 2, TradeRequest({ sell: 0x2e234DAe75C793f67A35089C9d99245E1C58470b, buy: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, sellAmount: 10000000000000000000 [1e19], minBuyAmount: 5000000000000000000 [5e18] })) │ └─ ← [Revert] revert: Invalid trade state └─ ← [Stop] Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.15ms (299.71µs CPU time) ``` Thus the `GnosisTrade::init` function reverts since the `GnosisTrade::stateTransition` modifier expects `TradeStatus == NOT_STARTED` whereas `TradeStatus` was set to `CLOSED` in the constructor at deployment.

Tools Used

Manual Review

Foundry

Recommended Mitigation Steps

1. Consider changing the input parameter of the GnosisTrade::stateTransition modifier passed to the GnosisTrade::init function from NOT_STARTED to CLOSED

   
   function init(
       IBroker broker_,
       address origin_,
       IGnosis gnosis_,
       uint48 batchAuctionLength,
       TradeRequest calldata req
   -   ) external stateTransition(TradeStatus.NOT_STARTED, TradeStatus.OPEN) {
   +   ) external stateTransition(TradeStatus.CLOSED, TradeStatus.OPEN) {

2. Alternatively, you may consider changing `TradeStatus` in the constructor function from `CLOSED` to `NOT_STARTED`

```diff
    constructor() {
-        status = TradeStatus.CLOSED;
+        status = TradeStatus.CLOSED;
    }

Assessed type

Other