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

Handling of Empty Data Arrays in StakingPool Causes Array Out-of-Bounds Access

galturok

Summary

The StakingPool contract's _depositLiquidity function, which fails to properly handle scenarios where the internal system logic sets the data array to be empty. This results in an array out-of-bounds access that can cause unexpected transaction reverts. This issue arises not from user error, but from the contract itself during operational scenarios where using an empty data array is valid and expected.

Vulnerability Details

The _depositLiquidity function iterates over the list of strategies to distribute deposited tokens while attempting to pass elements of the _data array to each strategy's deposit method.

https://github.com/stakedotlink/contracts/blob/5070f79cafc3604c7c6972c38453c03f69633cdb/contracts/core/StakingPool.sol#L477C1-L492C6

function _depositLiquidity(bytes[] calldata _data) private {
uint256 toDeposit = token.balanceOf(address(this));
if (toDeposit > 0) {
@>> for (uint256 i = 0; i < strategies.length; i++) {
IStrategy strategy = IStrategy(strategies[i]);
uint256 strategyCanDeposit = strategy.canDeposit();
if (strategyCanDeposit >= toDeposit) {
@>> strategy.deposit(toDeposit, _data[i]);
break;
} else if (strategyCanDeposit > 0) {
strategy.deposit(strategyCanDeposit, _data[i]);
toDeposit -= strategyCanDeposit;
}
}
}
}
​

However, the function mistakenly assumes that the _data array will always be populated with entries corresponding to each strategy. As seen in the FundFlowController.sol, there are specific cases where this contract sets the data array to be empty intentionally:

getDepositData from FundFlowController.sol may return an empty data array as valid output under certain conditions, such as when there are no vaults or when allocations do not fill up the expected slots.

The VaultControllerStrategy._depositToVaults allows for empty data to pass the revert ckeck, and will " deposit into additional vaults that don't yet belong to a group".

https://github.com/stakedotlink/contracts/blob/5070f79cafc3604c7c6972c38453c03f69633cdb/contracts/linkStaking/base/VaultControllerStrategy.sol#L172C1-L292C6

function _depositToVaults(
uint256 _toDeposit,
uint256 _minDeposits,
uint256 _maxDeposits,
uint64[] memory _vaultIds
) private returns (uint256) {
uint256 toDeposit = _toDeposit;
uint256 totalRebonded;
GlobalVaultState memory globalState = globalVaultState;
VaultGroup[] memory groups = vaultGroups;
​
// deposits must continue with the vault they left off at during the previous call
@>> if (_vaultIds.length != 0 && _vaultIds[0] != globalState.groupDepositIndex)
revert InvalidVaultIds();
​
//......
​
@>> // deposit into additional vaults that don't yet belong to a group
uint256 numVaults = vaults.length;
uint256 i = globalState.depositIndex;
​
while (i < numVaults) {
IVault vault = vaults[i];
uint256 deposits = vault.getPrincipalDeposits();
uint256 canDeposit = _maxDeposits - deposits;
​
// cannot leave a vault with less than minimum deposits
if (deposits < _minDeposits && toDeposit < (_minDeposits - deposits)) {
break;
}
​
if (toDeposit > canDeposit) {
vault.deposit(canDeposit);
toDeposit -= canDeposit;
} else {
vault.deposit(toDeposit);
if (toDeposit < canDeposit) {
toDeposit = 0;
break;
}
toDeposit = 0;
}
​
++i;
}
​
globalVaultState.depositIndex = uint64(i);
​
return _toDeposit - toDeposit;
}
​

Impact

When an empty data array is expected internally by the system's own functions, the StakingPool faces a logic disconnect that results in array out-of-bounds access and transaction reverts.

POC

In our Proof of Concept (PoC) setup, we will focus on recreating and demonstrating the specific vulnerability associated with handling empty data arrays within the StakingPool contract. Although there are multiple pathways for deposits, including normal deposits, performUpkeep, and handling of queued tokens, each capable of leading to the deposit function invocation, our test will specifically target the normal deposit process. By employing a direct deposit call with an empty data array, we aim to precisely trigger the array out-of-bounds access error. This method allows us to isolate the issue within the _depositLiquidity function without the complexity introduced by other operational pathways.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
​
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import "../lib/forge-std/src/Test.sol";
import "../contracts/core/priorityPool/PriorityPool.sol";
import "../contracts/core/priorityPool/WithdrawalPool.sol";
import "../contracts/core/StakingPool.sol";
import "./MockERC20.sol";
import "./SDLPoolMock.sol";
import "./LSTMock.sol";
import "./StrategyMock.sol";
​
​
​
contract PriorityPoolTest is Test {
PriorityPool public priorityPool;
PriorityPool public logic;
ERC1967Proxy public priorityPoolproxy;
ERC1967Proxy public withdrawalPoolproxy;
ERC1967Proxy public stakingPoolProxy;
ERC1967Proxy public strategyMockProxy;
MockERC20 public mockToken;
LSTMock public lstMock;
WithdrawalPool public withdrawalPool;
StakingPool public stakingPool;
SDLPoolMock public sdlPool;
StrategyMock public strategyMock;
​
​
address public user = address(25);
​
address public feeReceiver1 = address(0xBEE1);
address public feeReceiver2 = address(0xBEE2);
uint256 public unusedDepositLimit = 5 ether;
​
function setUp() public {
​
mockToken = new MockERC20("Mock Token", "MOCK", 1000 ether);
​
sdlPool = new SDLPoolMock();
​
lstMock = new LSTMock("LST Mock", "LST", 1000 ether);
​
logic = new PriorityPool(); // Deploy the implementation contract
​
// Define the fees
StakingPool.Fee[] memory fees = new StakingPool.Fee[]();
fees[0] = StakingPool.Fee({receiver: feeReceiver1, basisPoints: 100});
fees[1] = StakingPool.Fee({receiver: feeReceiver2, basisPoints: 200});
​
WithdrawalPool withdrawalPoolLogic = new WithdrawalPool();
StakingPool stakingPoolLogic = new StakingPool();
StrategyMock strategyMockLogic = new StrategyMock();
​
​
stakingPoolProxy = new ERC1967Proxy(
address(stakingPoolLogic),
abi.encodeWithSelector(
StakingPool.initialize.selector,
address(mockToken), // Mock ERC20 token address
"stakingToken",
"SKT",
fees, // _fees
unusedDepositLimit // _unusedDepositLimit
)
);
​
stakingPool = StakingPool(address(stakingPoolProxy));
​
strategyMockProxy = new ERC1967Proxy(
address(strategyMockLogic),
abi.encodeWithSelector(
StrategyMock.initialize.selector ,
address(mockToken),
address(stakingPool),
100 ether,
5 ether
)
);
​
strategyMock = StrategyMock(address(strategyMockProxy));
​
// Deploy proxy with initialize function
priorityPoolproxy = new ERC1967Proxy(
address(logic),
abi.encodeWithSelector(
PriorityPool.initialize.selector,
address(mockToken), // Mock ERC20 token address
stakingPool,
sdlPool,
uint128(1 ether), // queueDepositMin
uint128(10 ether) // queueDepositMax
)
);
​
// Cast the proxy to PriorityPool
priorityPool = PriorityPool(address(priorityPoolproxy));
​
withdrawalPoolproxy = new ERC1967Proxy(
address(withdrawalPoolLogic),
abi.encodeWithSelector(
WithdrawalPool.initialize.selector,
address(mockToken), // Mock ERC20 token address
address(stakingPool),
address(priorityPool),
uint256(1 ether), // min withdraw
uint64(1000) // time between withdrawals
)
);
​
withdrawalPool = WithdrawalPool(address(withdrawalPoolproxy));
​
// Set Withdrawal Pool
priorityPool.setWithdrawalPool(address(withdrawalPool));
​
stakingPool.addStrategy(address(strategyMock));
stakingPool.setPriorityPool(address(priorityPool));
}
​
​
​
function testDepositTokensEmptyData() public {
uint256 initialBalance = mockToken.balanceOf(user);
​
// Transfer some tokens to user
mockToken.transfer(user, 100 ether);
​
// Approve PriorityPool to spend user's tokens
vm.startPrank(user);
mockToken.approve(address(priorityPool), 100 ether);
​
// Deposit tokens into the PriorityPool to chech impact of empty data
priorityPool.deposit(50 ether, true, new bytes[](0));
​
vm.stopPrank();
}
​
}
​

Logs

​
[FAIL. Reason: panic: array out-of-bounds access (0x32)] testDepositTokensEmptyData() (gas: 200325)
Traces:
[200325] PriorityPoolTest::testDepositTokensEmptyData()
├─ [2562] MockERC20::balanceOf(0x0000000000000000000000000000000000000019) [staticcall]
│ └─ ← [Return] 0
├─ [27834] MockERC20::transfer(0x0000000000000000000000000000000000000019, 100000000000000000000 [1e20])
│ ├─ emit Transfer(from: PriorityPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000019, value: 100000000000000000000 [1e20])
│ └─ ← [Return] true
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000019)
│ └─ ← [Return]
├─ [24624] MockERC20::approve(ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 100000000000000000000 [1e20])
│ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000019, spender: ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], value: 100000000000000000000 [1e20])
│ └─ ← [Return] true
├─ [128444] ERC1967Proxy::deposit(50000000000000000000 [5e19], true, [])
│ ├─ [123527] PriorityPool::deposit(50000000000000000000 [5e19], true, []) [delegatecall]
│ │ ├─ [27722] MockERC20::transferFrom(0x0000000000000000000000000000000000000019, ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 50000000000000000000 [5e19])
│ │ │ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000019, spender: ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], value: 50000000000000000000 [5e19])
│ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000019, to: ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], value: 50000000000000000000 [5e19])
│ │ │ └─ ← [Return] true
│ │ ├─ [19688] ERC1967Proxy::getTotalQueuedWithdrawals() [staticcall]
│ │ │ ├─ [14793] WithdrawalPool::getTotalQueuedWithdrawals() [delegatecall]
│ │ │ │ ├─ [7384] ERC1967Proxy::getStakeByShares(0) [staticcall]
│ │ │ │ │ ├─ [2486] StakingPool::getStakeByShares(0) [delegatecall]
│ │ │ │ │ │ └─ ← [Return] 0
│ │ │ │ │ └─ ← [Return] 0
│ │ │ │ └─ ← [Return] 0
│ │ │ └─ ← [Return] 0
│ │ ├─ [17977] ERC1967Proxy::canDeposit() [staticcall]
│ │ │ ├─ [17582] StakingPool::canDeposit() [delegatecall]
│ │ │ │ ├─ [7298] ERC1967Proxy::getMaxDeposits() [staticcall]
│ │ │ │ │ ├─ [2403] StrategyMock::getMaxDeposits() [delegatecall]
│ │ │ │ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ │ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ ├─ [40759] ERC1967Proxy::deposit(0x0000000000000000000000000000000000000019, 50000000000000000000 [5e19], [])
│ │ │ ├─ [40342] StakingPool::deposit(0x0000000000000000000000000000000000000019, 50000000000000000000 [5e19], []) [delegatecall]
│ │ │ │ ├─ [2562] MockERC20::balanceOf(ERC1967Proxy: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]) [staticcall]
│ │ │ │ │ └─ ← [Return] 0
│ │ │ │ ├─ [25498] MockERC20::transferFrom(ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], ERC1967Proxy: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], 50000000000000000000 [5e19])
│ │ │ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], to: ERC1967Proxy: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], value: 50000000000000000000 [5e19])
│ │ │ │ │ └─ ← [Return] true
│ │ │ │ ├─ [562] MockERC20::balanceOf(ERC1967Proxy: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]) [staticcall]
│ │ │ │ │ └─ ← [Return] 50000000000000000000 [5e19]
│ │ │ │ ├─ [3150] ERC1967Proxy::canDeposit() [staticcall]
│ │ │ │ │ ├─ [2755] StrategyMock::canDeposit() [delegatecall]
│ │ │ │ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ │ │ │ └─ ← [Return] 100000000000000000000 [1e20]
│ │ │ │ └─ ← [Revert] panic: array out-of-bounds access (0x32)
│ │ │ └─ ← [Revert] panic: array out-of-bounds access (0x32)
│ │ └─ ← [Revert] panic: array out-of-bounds access (0x32)
│ └─ ← [Revert] panic: array out-of-bounds access (0x32)
└─ ← [Revert] panic: array out-of-bounds access (0x32)

Tools Used

Foundry

Recommendations

Further research into related contracts and interactions beyond the immediate scope may be necessary to develop a comprehensive fix for this issue. However, as a possible immediate fix, the following adjustment can be made to allow the _depositLiquidity function to handle empty data arrays gracefully.

function _depositLiquidity(bytes[] calldata _data) private {
uint256 toDeposit = token.balanceOf(address(this));
if (toDeposit > 0) {
for (uint256 i = 0; i < strategies.length; i++) {
IStrategy strategy = IStrategy(strategies[i]);
uint256 strategyCanDeposit = strategy.canDeposit();
​
// Check if _data array has an entry for the current strategy, use empty data if not
bytes memory dataForStrategy = i < _data.length ? _data[i] : new bytes(0);
​
if (strategyCanDeposit >= toDeposit) {
strategy.deposit(toDeposit, dataForStrategy);
break;
} else if (strategyCanDeposit > 0) {
strategy.deposit(strategyCanDeposit, dataForStrategy);
toDeposit -= strategyCanDeposit;
}
}
}
}
​

Retest after code fix

Logs

Ran 1 test for test/poc1emptyData.t.sol:PriorityPoolTest
[PASS] testDepositTokensEmptyData() (gas: 306378)
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

_depositToVaults can be feed empty data by internal functions and revert

Support

FAQs

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