FairSide contest - gzeon's results

Max 3 wallets exist currently for one membership. They are: that of the msg.sender plus the additional two in case they are needed, leading to an invalid exhibit.

Based on my interpretation, the warden wanted to say that the "require" statement for wallets length is not really necessary as the array cannot exceed 2 elements (I think the warden mistakenly wrote 3).

Marking this as a duplicate of #56

Comments do not pose any actual threat, so severity should be lowered to level zero.

A duplicate of #41.

Each action will change the signature, as the action is an input of the signature creation.

I think the warden wanted to say that the same signature can be used again to trigger this branch and repeatedly set csr.votingOpen or csr.evidence:

 } else if (
      action == Action.SubmitEvidence || action == Action.ExtendClaim
  ) {
      require(
          _isApprovedByAssessors(sig, id, action),
          "FSDNetwork::updateCostShareRequest: Insufficient Privileges"
      );
      CostShareRequest storage csr = costShareRequests[id];
      csr.votingOpen = uint128(block.timestamp);
      if (action == Action.SubmitEvidence) {
          // Should represent hash of evidence
          csr.evidence = abi.decode(data, (bytes32));
      }
  }

While the action is part of the signature, here it will be the same action, so the signature will not change. Usually, it is a good practice to increment nonces, so the same signature cannot be replicated.

I will leave this issue as of Medium severity, although the current impact cannot be estimated as the warden mentioned it lacks the implementation and no usage of 'votingOpen' or 'evidence' is present in the codebase.

Also, by looking at this updateCostShareRequest function I have noticed something from myself:

     * Requirements:
     * - only callable by governance or timelock contracts.

However, this function contains no auth checks I expected to see:

  require(
      msg.sender == GOVERNANCE_ADDRESS || msg.sender == TIMELOCK,
      "FSDNetwork::setSlippageTolerance: Insufficient Privileges"
  );

Anyone can call it supposed they have a valid signature. Because evidence data is not part of the signature, anyone can pass any arbitrary value. Make sure that's the intended behavior.

Thank you for looking into it. The comment is leftover from the original implementation. With regards to the finding, assessors can be voted on and off by the DAO. As a result, this is a non-critical finding.

Despite voting off the malicious accessor, I think the issue remains. The function does not have any auth check thus a signature for the aforementioned actions can be replicated by anyone else, not only accessor.

If you inspect the _isApprovedByAssessors function, you will see that the caller is mandated to be an assessor. As such, it cannot be replicated by anyone else. Additionally, the issue only manifests when the malicious assessor is the one that did not sign the payload. In general, its a privileged only function and voting off assessors is a protocol feature. Otherwise, malicious assessors could harm the system to a great degree

You are right, this function is only accessible by assessors. However, the issue is still present, even not that significant, so I am lowering the severity to 'Low'.