Potential DOS during swap
Description
The swap function iterates through the entire array of issues. These issues are created upon performing a stake() operation, and they essentially represent a user deposit.
Because there is no minimal cap as to the amount of tokens to stake() in the pool, a malicious user could perform stake() with one unit of tokens multiple times, which would essentially clog the issues array with a lot of entries, severely impairing the for loop performed in the swap function:
for (uint i = indexEnd; i > indexStar; i--) {
Issue storage issueInfo = issues[i];
if (issueInfo.isStaking) {
if (amountB > 0) {
if (amountB >= issueInfo.issueAmount) {
uint amountA = (issueInfo.issueAmount * 10000) / rate;
amountB -= issueInfo.issueAmount;
redeemToekn.safeTransfer(issueInfo.user, amountA);
issueInfo.isStaking = false;
} else {
uint amountA = (amountB * 10000) / rate;
redeemToekn.safeTransfer(issueInfo.user, amountA);
issueInfo.issueAmount -= amountB;
amountB = 0;
}
}
}
}Impact
The lack of adequate checks in stake() could potentially lead to a denial of service (DOS) for the StakingPool contract, as a malicious user could clog the system with a lot of small stake operations, which would make the swap function perform a lot of unnecessary iterations, eventually reaching the gas limit and rendering the pool unusable.
Recommendations
We recommend implementing a minimum cap for the amount of tokens that can be staked in the pool. This would limit the amount of issues that could be created and would prevent a potential DOS attack. Additionally, we recommend reevaluating the swap function to extract the necessary liquidity in various ways, such as using a more efficient algorithm or by using a more dynamic approach to determining which issues should be considered for the swap.
On top of the aforementioned mitigation directions, we recommend implementing a break condition should amountB be 0 to prevent unnecessary iterations.
for (uint i = indexEnd; i > indexStar; i--) {
Issue storage issueInfo = issues[i];
if (issueInfo.isStaking) {
if (amountB > 0) {
// ...
+ } else {
+ break;
+ }
}
}Remediation
This issue has been acknowledged by Chateau Capital, and a fix was implemented in commit 8184d6c5↗.