Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/trading/GnosisTrade.sol#L59-L65

Vulnerability details

Impact

Lack of reentrancy protection in the stateTransition modifier occurs in the following line of code.

modifier stateTransition(TradeStatus begin, TradeStatus end) {
        require(status == begin, "Invalid trade state");
        status = TradeStatus.PENDING;
        _;
        assert(status == TradeStatus.PENDING);
        status = end;
    }

In the GnosisTrade smart contract, the stateTransition modifier is used to enforce the state-machine pattern and to protect against reentrancy, however, it does not have any code that prevents reentrancy by external contracts calling the same function simultaneously.

As you can see there is no code that will prevent reentrancy while the function is executing.

For example, an attacker could call the init() function, and while the init() function is still executing, the attacker could call the init() function again. This could cause the smart contract to set the state variables multiple times or cause the smart contract to initiate multiple auctions with the same parameters, which could lead to unexpected behavior and potential loss of funds.

Proof of Concept

pragma solidity ^0.8.9;

import "./GnosisTrade.sol";

contract Attacker {
    GnosisTrade gnosisTrade;

    function attack(address gnosisTradeAddress) public {
        gnosisTrade = GnosisTrade(gnosisTradeAddress);
        //call the init function
        gnosisTrade.init();
        //call the init function again before the first call is completed 
        gnosisTrade.init();
    }
}

In this example, the Attacker contract is calling the init() function of the GnosisTrade contract twice, before the first call is completed. Since the stateTransition modifier does not have any code that prevents reentrancy, the state of the GnosisTrade contract could be modified multiple times in quick succession, which could cause unexpected behavior such as initiating multiple auctions with the same parameters or losing funds.

Tools Used

Manual audit.

Recommended Mitigation Steps

One of the most common ways to prevent reentrancy is to use a mutex, which is a flag variable that indicates whether the smart contract is currently executing a call. The init() function should check the state of the mutex before executing, and if it's already set, the function should immediately return without doing anything.

bool mutex = false;
modifier stateTransition(TradeStatus begin, TradeStatus end) {
        require(status == begin, "Invalid trade state");
        require(!mutex, "Reentrancy detected");
        mutex = true;
        _;
        mutex = false;
        status = end;
    }

Or use a guard condition, which is a condition that prevents the smart contract from executing the same call twice. The guard condition can be implemented by using the block number, timestamp, or any other unique value that changes for each call.

uint256 guard;
modifier stateTransition(TradeStatus begin, TradeStatus end) {
        require(status == begin, "Invalid trade state");
        require(guard != block.timestamp, "Reentrancy detected");
        guard = block.timestamp;
        _;
        status = end;
    }

Instead of implementing reentrancy protection manually, a reentrancy guard library such as OpenZeppelin's ReentrancyGuard can be used. This library provides a nonReentrant modifier that can be applied to functions to prevent them from being called again while they are already executing.

code-423n4 / 2023-01-reserve-findings