The contract BackingManager.sol may potentially contain a reentrancy vulnerability due to the lack of usage of reentrancy guards in the settleTrade method.
If the settleTrade method is called by another contract, it calls the rebalance function, which in turn makes a call to an external contract.
An attacker can potentially manipulate the state changes at this moment, facilitating a reentrancy attack.
In the settleTrade() function here, there is a potential spot for reentrancy attack when executing the function from another contract:
function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
...
// 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
}
}
}
Tools Used
Manual Review
Recommended Mitigation Steps
Appropriate measures should be taken to ensure that no state changes can occur during external calls.
The rebalance function call should be moved to the end of the calling function to ensure no external state changes can affect the contract.
The OpenZeppelin library provides a modifier known as nonReentrant that can be used to guard against reentrancy attacks.
This modifier should be used on public or external methods that alter the contract's state.
Lines of code
https://github.com/reserve-protocol/protocol/blob/master/contracts/p1/BackingManager.sol#L85-L99
Vulnerability details
Impact
The contract BackingManager.sol may potentially contain a reentrancy vulnerability due to the lack of usage of reentrancy guards in the
settleTrademethod.If the
settleTrademethod is called by another contract, it calls therebalancefunction, which in turn makes a call to an external contract.An attacker can potentially manipulate the state changes at this moment, facilitating a reentrancy attack.
Proof of Concept
https://github.com/reserve-protocol/protocol/blob/master/contracts/p1/BackingManager.sol#L85-L99
In the
settleTrade()function here, there is a potential spot for reentrancy attack when executing the function from another contract:Tools Used
Manual Review
Recommended Mitigation Steps
Appropriate measures should be taken to ensure that no state changes can occur during external calls.
The
rebalancefunction call should be moved to the end of the calling function to ensure no external state changes can affect the contract.The OpenZeppelin library provides a modifier known as
nonReentrantthat can be used to guard against reentrancy attacks.This modifier should be used on public or external methods that alter the contract's state.
Assessed type
Reentrancy