dimi6oni - Unbounded loop in `SophonFarming::massUpdatePools` leads to potential Denial of Service (DoS)

[sherlock-admin4]

dimi6oni

high

Unbounded loop in SophonFarming::massUpdatePools leads to potential Denial of Service (DoS)

Summary

The massUpdatePools function contains an unbounded loop that can exceed the block gas limit if the number of pools is large, potentially causing a denial of service (DoS).

Vulnerability Detail

The massUpdatePools function in the SophonFarming contract is designed to update the state of all pools by calling the updatePool function for each pool in the poolInfo array. This function contains an unbounded loop that iterates over all pools, which can become problematic as the number of pools increases. If the number of pools is large, the gas required to execute this function may exceed the block gas limit, causing the transaction to fail.

Impact

If the transaction fails due to running out of gas, the massUpdatePools function will not be able to update the pool states, leading to stale data and incorrect reward calculations. This can disrupt the functionality of the protocol, as users may not receive the correct amount of rewards. In a worst-case scenario, this could result in a denial of service (DoS) where no pool updates can be performed, severely impacting the protocol's operations and user trust.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L399-L405

Tool used

Manual Review

Recommendation

To mitigate this issue, implement pagination in the massUpdatePools function. This means updating the pools in smaller batches rather than all at once. By doing so, each call to massUpdatePools will handle a subset of the pools, ensuring that the gas limit is not exceeded and allowing the function to complete successfully. Additionally, consider adding mechanisms to track the last updated pool and continue from there in subsequent calls. This can help distribute the gas cost over multiple transactions and prevent a single transaction from running out of gas.

Here is the remediated code for the massUpdatePools function with pagination to prevent running out of gas:

+ uint256 public lastUpdatedPoolId = 0;
+ uint256 public constant BATCH_SIZE = 10; // Adjust the batch size as needed

function massUpdatePools() public {
    uint256 length = poolInfo.length;
+   uint256 startPoolId = lastUpdatedPoolId;
+   uint256 endPoolId = startPoolId + BATCH_SIZE > length ? length : startPoolId + BATCH_SIZE;

+   for (uint256 pid = startPoolId; pid < endPoolId; pid++) {
+       updatePool(pid);
+   }

-       for(uint256 pid = 0; pid < length;) {
-       updatePool(pid);
-       unchecked { ++pid; }
-   }

    // Update the last updated pool ID
+   lastUpdatedPoolId = endPoolId;

    // Reset lastUpdatedPoolId if all pools have been updated
+   if (lastUpdatedPoolId >= length) {
+       lastUpdatedPoolId = 0;
+   }
}

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

invalid because the array length is controlled by the trusted admin/owner. Currently, there are only 3 elements in poolInfo