Category: Coding Mistakes
No length check is present on the amounts parameter
Informational Severity
Informational Impact
N/A Likelihood
Description
The depositQueuedTokens and forceUnbond functions both take in an _amounts array that is expected to match the length of the _vaultIds array:
function depositQueuedTokens(
uint256[] calldata _vaultIds,
uint256[] calldata _amounts
) external onlyFundFlowController {
// [...]
for (uint256 i = 0; i < _vaultIds.length; ++i) {
if (_vaultIds[i] == skipIndex) revert InvalidVaultIds();
uint256 amount = _amounts[i];
if (amount == 0) revert InvalidAmount();
vaults[_vaultIds[i]].deposit(amount);
}
// [...]
}
function forceUnbond(
uint256[] calldata _vaultIds,
uint256[] calldata _amounts
) external onlyFundFlowController {
// [...]
for (uint256 i = 0; i < _vaultIds.length; ++i) {
if (_vaultIds[i] == skipIndex) revert InvalidVaultIds();
if (i > 0 && _vaultIds[i] <= _vaultIds[i - 1]) revert InvalidVaultIds();
if (_amounts[i] == 0) revert InvalidAmount();
vaults[_vaultIds[i]].unbond(_amounts[i]);
totalUnbonded += _amounts[i];
}
// [...]
}However, there is no length check in either function.
Impact
If the _amounts parameter is longer than the _vaultIds parameter, then the extra entries will be disregarded. If it is shorter, then the above code will revert due to the array's bounds check. If the lengths do not match, then the calldata is malformed.
Recommendations
We recommend adding a length check to both these functions so that the erroneous case where the _amounts parameter is longer than the _vaultIds parameter fails noisily instead of quietly succeeding.
Remediation
This issue has been acknowledged by Stake.link, and a fix was implemented in commit 337298aa↗.