Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Redundant token transfers causes loss of funds

Dup1337

Summary

In the PriorityPool contract, there is an inefficient and potentially user-unfriendly behavior in the deposit function and its internal _deposit function.

When a user attempts to deposit tokens without queuing (_shouldQueue is false), and the system cannot process the entire deposit immediately, the remaining tokens are transferred back to the user instead of reverting the transaction.

This can lead to unnecessary gas expenditure for the user, as they pay gas fees for transferring tokens to the contract and then back to themselves, without achieving the intended deposit. It is because the contract does not perform a preliminary check to determine if the deposit can be fully processed, and it lacks an option to revert the transaction when the deposit cannot be completed as desired.

Vulnerability Details

The user calls the deposit function with the desired _amount, a boolean _shouldQueue indicating whether to queue the deposit if it cannot be processed immediately, and _data for additional parameters;

Contract: PriorityPool.sol
​
254: function deposit(uint256 _amount, bool _shouldQueue, bytes[] calldata _data) external {
255: if (_amount == 0) revert InvalidAmount();
256: token.safeTransferFrom(msg.sender, address(this), _amount);
257: _deposit(msg.sender, _amount, _shouldQueue, _data);
258: }

And here is the _deposit function logic;

Contract: PriorityPool.sol
​
601: function _deposit(
602: address _account,
603: uint256 _amount,
604: bool _shouldQueue,
605: bytes[] memory _data
606: ) internal {
607: if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
608:
609: uint256 toDeposit = _amount;
610:
611: if (totalQueued == 0) {
612: uint256 queuedWithdrawals = withdrawalPool.getTotalQueuedWithdrawals();
613: if (queuedWithdrawals != 0) {
614: uint256 toDepositIntoQueue = toDeposit <= queuedWithdrawals
615: ? toDeposit
616: : queuedWithdrawals;
617: withdrawalPool.deposit(toDepositIntoQueue);
618: toDeposit -= toDepositIntoQueue;
619: IERC20Upgradeable(address(stakingPool)).safeTransfer(_account, toDepositIntoQueue);
621:
622: if (toDeposit != 0) {
623: uint256 canDeposit = stakingPool.canDeposit();
624: if (canDeposit != 0) {
625: uint256 toDepositIntoPool = toDeposit <= canDeposit ? toDeposit : canDeposit;
626: stakingPool.deposit(_account, toDepositIntoPool, _data);
627: toDeposit -= toDepositIntoPool;
628: }
629: }
630: }
631:
632: if (toDeposit != 0) {
633: if (_shouldQueue) {
634: _requireNotPaused();
635: if (accountIndexes[_account] == 0) {
636: accounts.push(_account);
637: accountIndexes[_account] = accounts.length - 1;
638: }
639: accountQueuedTokens[_account] += toDeposit;
640: totalQueued += toDeposit;
641: } else {
642: >>> token.safeTransfer(_account, toDeposit);
643: }
644: }
645:
646: emit Deposit(_account, _amount - toDeposit, _shouldQueue ? toDeposit : 0);
647: }
​

The _deposit function starts by checking if the pool status is open.
It initializes toDeposit with the _amount.

  • If there are no queued deposits (totalQueued == 0), it checks for any queued withdrawals. (L:611)

  • If there are queued withdrawals, it deposits tokens into the withdrawal pool and transfers staking tokens back to the user. (L:613)

  • The amount deposited into the withdrawal pool is up to the amount of queued withdrawals.

Depositing into the Staking Pool:

  • If there is still toDeposit remaining, it attempts to deposit into the staking pool. (L:622)

  • It checks how much the staking pool can accept (canDeposit).

  • It deposits as much as possible into the staking pool and reduces toDeposit.

Handling Remaining toDeposit:

  • If there is still toDeposit remaining after attempting to deposit into the withdrawal and staking pools:

And If _shouldQueue is false (our case), it transfers the remaining tokens back to the user (line 642).

So it´s possible that totalQueued != 0 (skips L:611) , _shouldQueue is false (Skip L:633) and the user lands on L:642 again receiving back their own tokens without any state changes.
Please be aware that this can easily occur organically especially at high peak times.

And since the users approve the contract for the funds being pulled from them (L:256) that would also cost another gas wastage assuming they approved only the _amount.

Impact

Unnecessary gas costs due to redundant token transfers

Tools Used

Manual Review

Recommendations

Create a function that allows users to check if their deposit can be fully processed before initiating the transfer.

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.