Skip to content

[SOV-4416] apply reentrancy guard zero #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: development
Choose a base branch
from

Conversation

cwsnt
Copy link
Contributor

@cwsnt cwsnt commented Dec 11, 2024

No description provided.

@tjcloa tjcloa self-requested a review December 18, 2024 01:30
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls sync from the target branch.
changes requested, see comments.

Comment on lines 161 to 163
/*
* @notice set the user's block number for opening, and do check & reset to 0 for closing
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* @notice set the user's block number for opening, and do check & reset to 0 for closing
*
/*
* @notice set the user's block number for opening or increasing LoCs, and do check & reset to 0 for closing or decreasing
*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 166 to 167
function notInTheSameBlockHandler(bool _isOpening) private {
if(_isOpening) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function notInTheSameBlockHandler(bool _isOpening) private {
if(_isOpening) {
function notInTheSameBlockHandler(bool _openOrIncrease) private {
if(_openOrIncrease) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else {
if(userBlockNumber[tx.origin] > 0) {
if(userBlockNumber[tx.origin] == block.number) {
revert("ZeroProtocolMutex: mutex locked");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
revert("ZeroProtocolMutex: mutex locked");
revert("Recovery mode mutex locked. Try in another block");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 40 to 44
/*
* to store the user's block number
*/
mapping(address => uint256) public userBlockNumber;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* to store the user's block number
*/
mapping(address => uint256) public userBlockNumber;
}
/*
* Store the LoC block number on open/increase when in the Recovery mode
*/
mapping(address => uint256) public recoveryModeMutex;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*
* @dev This is the function will be called by the open, close, increase, decrease trove function
*/
function notInTheSameBlockHandler(bool _isOpening) private {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function notInTheSameBlockHandler(bool _isOpening) private {
function recoveryModeMutexHandler(bool _isOpening) private {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjmr007 jjmr007 force-pushed the sov-4416-apply-reentrancy-guard-zero branch from 6b5079e to 9c5e883 Compare December 20, 2024 00:54
2 address comment review
IPriceFeedTestnet(_priceFeed).setPrice(1e8);

// repayZusd will affect(decrease) the debt, should revert due to reentrancy violation
borrowerOperations.repayZUSD(_ZUSDAmount, _upperHint, _lowerHint);
Copy link
Contributor

@tjcloa tjcloa Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test covers only one case: open trove - repay debt.
but is missing increase debt - repay.
and open trove - increase debt - repay debt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added unit test in this commit

@cwsnt cwsnt requested a review from tjcloa December 31, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants