klaus - fulfillRandomWords - may be reverted due to a hardcoded callbackGasLimit

[sherlock-admin2]

klaus

medium

fulfillRandomWords - may be reverted due to a hardcoded callbackGasLimit

Summary

The gas consumption can be changed by deployment parameter, but the callbackGasLimit is hardcoded.

This can cause fulfillRandomWords to revert if you change the deployment parameters.

Vulnerability Detail

The fulfillRandomWords function receives a random value and selects the agents to be healed, draws new wounded agents, and handles the agents that will die in this round.

The amount of gas fulfillRandomWords uses is depending on MAX_SUPPLY and AGENTS_TO_WOUND_PER_ROUND_IN_BASIS_POINTS. As MAX_SUPPLY or AGENTS_TO_WOUND_PER_ROUND_IN_BASIS_POINTS increases, more agents are wounded. These are not constant, but rather values that you set as parameters at deployment time.

Currently, when calling VRF_COORDINATOR.requestRandomWords, the callbackGasLimit is hardcoded as 2_500_000.

function _requestForRandomness() private {
  uint256 requestId = VRF_COORDINATOR.requestRandomWords({
      keyHash: KEY_HASH,
      subId: SUBSCRIPTION_ID,
      minimumRequestConfirmations: uint16(3),
@>    callbackGasLimit: uint32(2_500_000),
      numWords: uint32(1)
  });

By this code, you can check gas consumption of fulfillRandomWords with maximum number of users requesting heal(30). It is approximately 2_200_000. You can add it to the Infiltration.fulfillRandomWords.t.sol file and run it.

function test_gas() public {

  _startGameAndDrawOneRound();

  uint256[] memory randomWords = _randomWords();
  IInfiltration.HealResult[] memory healResults = new IInfiltration.HealResult[](30);

  for (uint256 roundId = 2; roundId <= ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD + 1; roundId++) {

      // request heal
      if(roundId == ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD + 1){
          uint256 healed;
          for(uint256 i = 2; healed < 30; i++){
              (uint256[] memory woundedAgentIdsFromRound, ) = infiltration.getRoundInfo({
                  roundId: uint40(roundId - i)
              });

              for(uint256 j; j < woundedAgentIdsFromRound.length; j ++){
                  uint256[] memory toHeal = new uint256[](1);

                  toHeal[0] = woundedAgentIdsFromRound[j];
                  healResults[healed].agentId = woundedAgentIdsFromRound[j];

                  _heal({roundId: roundId, woundedAgentIds: toHeal});

                  healed++;

                  if (healed >= 30){
                      break;
                  }
              }
          }
      }

      _startNewRound();

      // Just so that each round has different random words
      randomWords[0] += roundId;

      if (roundId == ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD + 1) {

          for(uint256 i; i < healResults.length; i++){
              if(healResults[i].agentId == 7249 || healResults[i].agentId == 5037){
                  healResults[i].outcome = IInfiltration.HealOutcome.Killed;
              }
          }
          expectEmitCheckAll();
          emit HealRequestFulfilled(roundId, healResults);

          (uint256[] memory woundedAgentIdsFromRound, ) = infiltration.getRoundInfo({
              roundId: uint40(roundId - ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD)
          });
          assertEq(woundedAgentIdsFromRound.length, 20);
          uint256[] memory woundedAgentIds = new uint256[](woundedAgentIdsFromRound.length);
          for (uint256 i; i < woundedAgentIdsFromRound.length; i++) {
              woundedAgentIds[i] = woundedAgentIdsFromRound[i];
          }

          expectEmitCheckAll();
          emit Killed(roundId - ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD, woundedAgentIds);
      }
      expectEmitCheckAll();
      emit RoundStarted(roundId + 1);

      uint256 requestId = _computeVrfRequestId(uint64(roundId));
      vm.prank(VRF_COORDINATOR);
      VRFConsumerBaseV2(address(infiltration)).rawFulfillRandomWords(requestId, randomWords);
  }
}

This test uses MAX_SUPPLY = 10000, AGENTS_TO_WOUND_PER_ROUND_IN_BASE_POINTS=20.

So this means that hardcoded callbackGasLimit , 2_500_000 is calculated based on MAX_SUPPLY = 10000, AGENTS_TO_WOUND_PER_ROUND_IN_BASE_POINTS=20.

| contracts/Infiltration.sol:Infiltration contract |                 |        |        |         |         |
|--------------------------------------------------|-----------------|--------|--------|---------|---------|
| Deployment Cost                                  | Deployment Size |        |        |         |         |
| 5036371                                          | 27886           |        |        |         |         |
| Function Name                                    | min             | avg    | median | max     | # calls |
| rawFulfillRandomWords                            | 496948          | 545114 | 520390 | 2180294 | 49      |

However, given that MAX_SUPPLY and AGENTS_TO_WOUND_PER_ROUND_IN_BASIS_POINTS are implemented to be changeable on deployment, so gasLimit should be computed accordingly.

If fulfillRandomWords is reverted due to low gas, chainlink integration can break, which can cause the protocol to stop.

Impact

The fulfillRandomWords callback function may be reverted, which may break chainlink integration.

Code Snippet

https://github.com/sherlock-audit/2023-10-looksrare/blob/86e8a3a6d7880af0dc2ca03bf3eb31bc0a10a552/contracts-infiltration/contracts/Infiltration.sol#L1298

Tool used

Manual Review

Recommendation

Instead of hardcoding the gasLimit, calculate it based on the variable and set it as immutable variable. Or, following chainlink's security recommendations, the callback only stores the random value, and any complex processing is handled by calling the function after the callback ends.

Duplicate of #136