Skip to content
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

Sherlock fix https://github.com/sherlock-audit/2023-03-Y2K-judging/issues/163 #125

Merged
merged 2 commits into from
May 10, 2023

Conversation

3xHarry
Copy link
Contributor

@3xHarry 3xHarry commented Apr 6, 2023

No description provided.

@3xHarry 3xHarry changed the base branch from main to earthquake-v2-sherlock-audit April 6, 2023 13:00
@3xHarry 3xHarry changed the base branch from earthquake-v2-sherlock-audit to sherlock-fix-1 April 6, 2023 13:03
Copy link

@IAm0x52 IAm0x52 left a comment

Choose a reason for hiding this comment

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

Needs additional changes. This will revert if diff is too high due to underflow in L412

@3xHarry
Copy link
Contributor Author

3xHarry commented May 6, 2023

@IAm0x52 position is validated to be in profit at this poiunt, meaning that entitledAmount will always be greater than shares.

// skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
if (queue[index].assets < relayerFee) {
index++;
continue;
}

uint256 originalDepositValue = queue[index].assets - diffInAssets;
Copy link

@IAm0x52 IAm0x52 May 6, 2023

Choose a reason for hiding this comment

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

@3xHarry Highly profitable epochs could underflow here since diffInAssets can be greater than queue[index].assets. I.e. entitled amount = 200 and assets = 99. diff = 101 and 99 - 101 will underflow. That edge case would break rollover functionality. Mitigation would be complex so maybe better to just acknowledge and accept the risk

Copy link
Contributor Author

@3xHarry 3xHarry May 6, 2023

Choose a reason for hiding this comment

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

@IAm0x52 issues does not exist in later PR, see permalink:

uint256 originalDepositValue = queue[index].shares -
previewAmountInShares(
queue[index].epochId,
(entitledAmount - queue[index].shares) // subtract profit from share value
);

Here i convert profit in nominal terms back to shares which is lower than nominal terms of entitledAmount
shares == entitledAmount

@3xHarry 3xHarry merged commit c283d5c into sherlock-fix-1 May 10, 2023
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