Potential malicious initialization of GnosisTrade and DutchTrade implementation contract

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/GnosisTrade.sol#L75 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/DutchTrade.sol#L100

Vulnerability details

Impact

Any erc20 assets transferred to the GnosisTrade or DutchTrade implementation contracts can be drained by a malicious user.

Proof of Concept

Just like the OpenZeppelin Initializable metioned:

 * [CAUTION]
 * ====
 * Avoid leaving a contract uninitialized.
 *
 * An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation
 * contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke
 * the {_disableInitializers} function in the constructor to automatically lock it when it is deployed:
 *
 * [.hljs-theme-light.nopadding]
 * ```
 * /// @custom:oz-upgrades-unsafe-allow constructor
 * constructor() {
 *     _disableInitializers();
 * }
 * ```
 * ====

The problem here is with the GnosisTrade implementation contract itself, not with its proxy contracts created by BrokerP1.newBatchAuction() (which are automatically initialized by the broker with an init call).

In reserve, the GnosisTrade contract is deployed once as an implementation contract(let's call it gnoImpl), and then lots of its proxy contracts will be deployed via BrokerP1.newBatchAuction() (let's call them gnoProxy1, gnoProxy2, etc). The proxy contracts(gnoProxy1, gnoProxy2, etc) will be initialized automatically by BrokerP1.newBatchAuction():

function newBatchAuction(TradeRequest memory req, address caller) private returns (ITrade) {
    require(batchAuctionLength > 0, "batch auctions not enabled");
    GnosisTrade trade = GnosisTrade(address(batchTradeImplementation).clone());
    ......
    trade.init(this, caller, gnosis, batchAuctionLength, req);
}

However, the gnoImpl contract was not initialized automatically in depoyment. As a result, a malicious use can steal all assets that other users have accidentally transferred to the gnoImpl (implementation) contract through calling init(), settle() and transferToOriginAfterTradeComplete().

The DutchTrade implementation contract has the same problem as the GnosisTrade implementation contract.

Tools Used

Manual Review

Recommended Mitigation Steps

We should add a constructor to GnosisTrade and DutchTrade contract, and set some flag to prevent the implementation contract being initialized maliciously after the deployment.

Assessed type

Other

[0xean]

@hihen - how do you believe this qualifies for M risk if the issue is predicated on a user mistakenly transferring assets to the this contract?

[tbrent]

Agree that this does not qualify for M. However, it does seem worth setting the status to TradeStatus.CLOSED in the constructor of the contract, so that the implementation contract is obviously bricked/unusable.

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)