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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
413b28a
fix https://github.com/sherlock-audit/2023-03-Y2K-judging/issues/163
3xHarry Apr 6, 2023
3732a70
fix https://github.com/sherlock-audit/2023-03-Y2K-judging/issues/75 (…
3xHarry May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/v2/Carousel/Carousel.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,35 +394,40 @@ contract Carousel is VaultV2 {
while ((index - prevIndex) < (_operations)) {
// only roll over if last epoch is resolved
if (epochResolved[queue[index].epochId]) {
uint256 entitledShares = previewWithdraw(
uint256 entitledAmount = previewWithdraw(
queue[index].epochId,
queue[index].assets
);
// mint only if user won epoch he is rolling over
if (entitledShares > queue[index].assets) {
if (entitledAmount > queue[index].assets) {
uint256 diff = entitledAmount - queue[index].assets;
// get diff amount in assets
uint256 diffInAssets = diff.mulDivUp(finalTVL[queue[index].epochId], claimTVL[queue[index].epochId]);
// 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 •
edited
Loading

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 •
edited
Loading

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:

Earthquake/src/v2/Carousel/Carousel.sol

Lines 422 to 426 in 5cb2890

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

// @note we know shares were locked up to this point
_burn(
queue[index].receiver,
queue[index].epochId,
queue[index].assets
originalDepositValue
);
// transfer emission tokens out of contract otherwise user could not access them as vault shares are burned
_burnEmissions(
queue[index].receiver,
queue[index].epochId,
queue[index].assets
originalDepositValue
);
// @note emission token is a known token which has no before transfer hooks which makes transfer safer
emissionsToken.safeTransfer(
queue[index].receiver,
previewEmissionsWithdraw(
queue[index].epochId,
queue[index].assets
originalDepositValue
)
);

Expand All @@ -431,8 +436,8 @@ contract Carousel is VaultV2 {
queue[index].receiver,
queue[index].receiver,
_epochId,
queue[index].assets,
entitledShares
originalDepositValue,
entitledAmount
);
uint256 assetsToMint = queue[index].assets - relayerFee;
_mintShares(queue[index].receiver, _epochId, assetsToMint);
Expand Down
7 changes: 7 additions & 0 deletions test/V2/Carousel/CarouselTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,9 @@ contract CarouselTest is Helper {

console.log("rollover queue length", vault.getRolloverQueueLenght());

// get value of prev epoch sahres for user
uint256 prevEpochShareValue = vault.previewWithdraw(prevEpoch, vault.balanceOf(USER, prevEpoch));

// mint rollovers again
// this should mint shares as prev epoch is in profit
// should only mint as many positions as there are in queue (6)
Expand All @@ -333,6 +336,10 @@ contract CarouselTest is Helper {
uint256 _relayerFee = (balanceAfter - balanceBefore) / 6;
assertEq(_relayerFee, relayerFee);

//@note after rollover, prev value of shares should subtract by original deposit value
uint256 prevEpochSharesValueAfterRollover = vault.previewWithdraw(prevEpoch, vault.balanceOf(USER, prevEpoch));
assertEq(((prevEpochSharesValueAfterRollover >> 1) << 1) , ((prevEpochShareValue - prevEpochUserBalance) >> 1) << 1); // zero out last bit to avoid rounding errors

// check balances
assertEq(vault.balanceOf(USER, _epochId), prevEpochUserBalance - relayerFee);
assertEq(vault.balanceOfEmissions(USER, _epochId), prevEpochUserBalance - relayerFee);
Expand Down
4 changes: 2 additions & 2 deletions test/V2/Helper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ contract Helper is Test {
uint256 public constant COLLATERAL_MINUS_FEES = 21989999998407999999;
uint256 public constant COLLATERAL_MINUS_FEES_DIV10 = 2198999999840799999;
uint256 public constant NEXT_COLLATERAL_MINUS_FEES = 21827317001456829250;
uint256 public constant USER1_EMISSIONS_AFTER_WITHDRAW = 1099999999999999999749;
uint256 public constant USER2_EMISSIONS_AFTER_WITHDRAW = 99999999999999999749;
uint256 public constant USER1_EMISSIONS_AFTER_WITHDRAW = 1096380172807730660741;
uint256 public constant USER2_EMISSIONS_AFTER_WITHDRAW = 96380172807730660741;
uint256 public constant USER_AMOUNT_AFTER_WITHDRAW = 13112658495641799945;
address public constant ADMIN = address(0x1);
address public constant WETH = address(0x888);
Expand Down