Medium Risk
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L56C1-L61C10
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L49C1-L52C10
PendingStakes and holders array which is stored as the state variables. If this arrays length extended to few hundreds it will potentially leads to reach the block.gasLimit and DOS.
Function getTstTotal() have for-loop is used to get total TST tokens by doing sum of array which leads to potential DOS if holders and pendingStakes array length have few hundreds.by calling all array data which is highly gas expensive it leads to hits the block.gaslimit in one transaction.
function getTstTotal() private view returns (uint256 _tst) { // Un-bound loop leads To gas DOS
for (uint256 i = 0; i < holders.length; i++) {
_tst += positions[holders[i]].TST;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
_tst += pendingStakes[i].TST;
}
}
In the above code we can see that two for-loops calls the holders and pendingStakes arrays in one transaction if two arrays have few hundreds length then it enough to make DOS by reaching block.gasLimit.
We noticed that protocol deploys on arbitrum but we can see block.gasLimit on arbitrum.
Before submitting this report the latest transaction made on arbitrum here :-
https://arbiscan.io/block/164636936
Arbitrum block gas limit :- 1,125,899,906,842,624
The simple swap transaction cost 4,227,935 on arbitrum.
Then few hundreds array length is enough to cause the DOS.
It will create DOS(denial of service) to subsequent requests.
Manual View
Add a mechanism which user can specify the min-max length of the array.
Below function which is also associated with DOS gas-limi issue getStakeTotal()
Function which is associated with above function also lands on DOS.
pendingstake-dos findings should be invalid. README states that It is a known issue. "vaults function array length is unchecked going into for loop. An abuse of NFT minting by a user could prevent them from being able to use this vaults function" "Length of accepted tokens array throughout contract is unchecked, but this Token Manager contract is managed by us and there is unlikely to be more than 5-10 tokens in this array" "No length check for number of stake holders. This could cause a problem throughout contract if there are a high number of stakers" "TokenManager getAcceptedTokens array length unchecked, but uses an administrative contract which is managed by us. Unlikely to be more than 5-10 items"
Thats entirely a different case, the known issue mentioned was with a different variable, the pendingStake variable is the vulnerability in question here. And the attack vector is entirely different. While I think the issue is there, this report doesn't highlight the actual attack vector and how an attacker can take advantage of it.