Any depositor can avoid paying deposit fee by substituting deposit(id, ...) call with an atomically coupled deposit(0, ...) -> mintDepositInQueue(id, 1) call.
Core reason is that deposit fee is fully substituted with relayer fee sent back to a caller, while depositQueue is processed in LIFO order.
Vulnerability Detail
Bob the depositor wants to deposit just before epoch start to minimize the level of risk by maximizing the information as of the deposit time.
To avoid paying nearly full deposit fee instead of deposit(id, _assets, Bob) he runs deposit(0, _assets, Bob) -> mintDepositInQueue(id, 1) atomically.
As relayer fee is sent back to msg.sender, Bob is effectively avoided paying deposit fee, having only somewhat increased the overall gas costs (even not so substantially, as deposit(0, ...) performs only one queue SSTORE instead of fee calculation and token transfer of direct deposit), which is negligible whenever Bob's deposit is big enough.
Impact
As such a replacement can be done for any deposit, protocol can lose up to all deposit fees.
Code Snippet
_deposit() do not apply the fee if the call is for zero epoch and depositQueue is being used:
function _deposit(
uint256 _id,
uint256 _assets,
address _receiver
) internal {
// mint logic, either in queue or direct deposit
if (_id != 0) {
uint256 assetsToDeposit = _assets;
if (depositFee > 0) {
(uint256 maxX, , uint256 minX) = getEpochConfig(_id);
// deposit fee is calcualted linearly between time of epoch creation and epoch starting (deposit window)
// this is because late depositors have an informational advantage
uint256 fee = _calculateFeePercent(int256(minX), int256(maxX));
// min minRequiredDeposit modifier ensures that _assets has high enough value to not devide by 0
// 0.5% = multiply by 10000 then divide by 50
uint256 feeAmount = _assets.mulDivDown(fee, 10000);
assetsToDeposit = _assets - feeAmount;
_asset().safeTransfer(treasury, feeAmount);
}
_mintShares(_receiver, _id, assetsToDeposit);
emit Deposit(msg.sender, _receiver, _id, _assets);
} else {
@> depositQueue.push(
QueueItem({assets: _assets, receiver: _receiver, epochId: _id})
);
emit DepositInQueue(msg.sender, _receiver, _id, _assets);
}
}
Then mintDepositInQueue() proceeds with the queue elements backwards, in LIFO order:
function mintDepositInQueue(uint256 _epochId, uint256 _operations)
external
epochIdExists(_epochId)
epochHasNotStarted(_epochId)
nonReentrant
{
// make sure there is already a new epoch set
// epoch has not started
QueueItem[] memory queue = depositQueue;
uint256 length = depositQueue.length;
// dont allow minting if epochId is 0
if (_epochId == 0) revert InvalidEpochId();
if (length == 0) revert OverflowQueue();
// relayers can always input a very big number to mint all deposit queues, without the need to read depostQueue length first
if (_operations > length) _operations = length;
// queue is executed from the tail to the head
// get last index of queue
@> uint256 i = length - 1;
while ((length - _operations) <= i) {
// this loop impelements FILO (first in last out) stack to reduce gas cost and improve code readability
// changing it to FIFO (first in first out) would require more code changes and would be more expensive
_mintShares(
queue[i].receiver,
_epochId,
queue[i].assets - relayerFee
);
emit Deposit(
msg.sender,
queue[i].receiver,
_epochId,
queue[i].assets - relayerFee
);
depositQueue.pop();
if (i == 0) break;
unchecked {
i--;
}
}
emit RelayerMinted(_epochId, _operations);
asset.safeTransfer(msg.sender, _operations * relayerFee);
}
Tool used
Manual Review
Recommendation
As an example, consider applying deposit fee on placing deposit queue request as well, i.e. do this part for all epoch ids:
if (depositFee > 0) {
(uint256 maxX, , uint256 minX) = getEpochConfig(_id);
// deposit fee is calcualted linearly between time of epoch creation and epoch starting (deposit window)
// this is because late depositors have an informational advantage
uint256 fee = _calculateFeePercent(int256(minX), int256(maxX));
// min minRequiredDeposit modifier ensures that _assets has high enough value to not devide by 0
// 0.5% = multiply by 10000 then divide by 50
uint256 feeAmount = _assets.mulDivDown(fee, 10000);
assetsToDeposit = _assets - feeAmount;
_asset().safeTransfer(treasury, feeAmount);
}
It looks it needs to be the full fee as otherwise the demonstrated approach can be able to lower it.
As there is no way to remove deposit from the queue and user will be placing deposits there in advance, the fee for them will not be high anyway.
iglyx
high
Deposit fee can always be avoided
Summary
Any depositor can avoid paying deposit fee by substituting
deposit(id, ...)
call with an atomically coupleddeposit(0, ...) -> mintDepositInQueue(id, 1)
call.Core reason is that deposit fee is fully substituted with relayer fee sent back to a caller, while
depositQueue
is processed inLIFO
order.Vulnerability Detail
Bob the depositor wants to deposit just before epoch start to minimize the level of risk by maximizing the information as of the deposit time.
To avoid paying nearly full deposit fee instead of
deposit(id, _assets, Bob)
he runsdeposit(0, _assets, Bob) -> mintDepositInQueue(id, 1)
atomically.As relayer fee is sent back to
msg.sender
, Bob is effectively avoided paying deposit fee, having only somewhat increased the overall gas costs (even not so substantially, asdeposit(0, ...)
performs only one queue SSTORE instead of fee calculation and token transfer of direct deposit), which is negligible whenever Bob's deposit is big enough.Impact
As such a replacement can be done for any deposit, protocol can lose up to all deposit fees.
Code Snippet
_deposit() do not apply the fee if the call is for zero epoch and
depositQueue
is being used:https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L470-L501
Then mintDepositInQueue() proceeds with the queue elements backwards, in LIFO order:
Tool used
Manual Review
Recommendation
As an example, consider applying deposit fee on placing deposit queue request as well, i.e. do this part for all epoch
id
s:https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L495
It looks it needs to be the full fee as otherwise the demonstrated approach can be able to lower it.
As there is no way to remove deposit from the queue and user will be placing deposits there in advance, the fee for them will not be high anyway.
Duplicate of #75