QA Report

[howlbot-integration]

See the markdown file with the details of this report here.

[3docSec]

L-1 makes sense, even though the borrower has the option to transfer tokens from another account and enter the condition currentlyHeld == totalDebts
L-2 good one, same as #90
L-3 I wouldn't count this one, it's the whole point of having a cap. Also the suggested reservation is a non-fix, because reservations could be frontrun too
L-4 OK
L-5 OK
L-6 I would not count this one either, makes little sense if we first don't assess that the authorized factory can be used to create insecure controllers
L-7 OK
L-8 The finding is invalid and the suggestion is harmful.

Try the following test:

  function test3doc() external {
    string memory testStr = hex'0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20';
    (bytes32 word0, bytes32 word1) = _packString(testStr);

    console.logBytes32(word0);
    console.logBytes32(word1);
  }


  function _packString(string memory str) internal pure returns (bytes32 word0, bytes32 word1) {
    assembly {
      let length := mload(str)
      // Equivalent to:
      // if (str.length > 63) revert NameOrSymbolTooLong();
      if gt(length, 0x3f) {
        mstore(0, 0x19a65cb6)
        revert(0x1c, 0x04)
      }
      // Load the length and first 31 bytes of the string into the first word
      // by reading from 31 bytes after the length pointer.
      word0 := mload(add(str, 0x1f))
      // If the string is less than 32 bytes, the second word will be zeroed out.
      word1 := mul(mload(add(str, 0x3f)), gt(mload(str), 0x1f))
    }
  }

Output with the current implementation is correct:

  0x200102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f
  0x2000000000000000000000000000000000000000000000000000000000000000

while the recommended fix breaks it:

  0x200102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f
  0x0000000000000000000000000000000000000000000000000000000000000000

L-9 I would not count this one, it's an NC
L-10 not sure on this one, but I'd rather err on the OK side
L-11 invalid, there's no checks or effects following interactions in the instance highlighted (at most you have two _callWith)
L-12 OK
L-13 I would not count this. It's at most a gas suggestion, and defense-in-depth is to be preferred
L-14 I would not count this. It's an opinionated "don't use assembly" suggestion
L-15 is harmful. gas() is the proper way to get the gas left in yul. The recommended mitigation does not compile.

Stopping here. As said in other QC reports, harmful recommendations are not welcome.

[c4-judge]

3docSec marked the issue as grade-c

[sathishpic22]

Hi @3docSec

Thank you for your feedback and valuable comments.

I understand that my grade was reduced to a C due to some of my suggestions and invalid findings potentially impacting the protocol. However, I believe it’s important to acknowledge that errors are an inherent aspect of the warden’s role. In this submission model, it’s impossible to submit only valid findings and suggestions. The process naturally involves a mixture of valid and invalid assessments, based on the warden’s understanding of the issue at the time. It is the judges who ultimately determine which findings are correct.

While I accept that some of my findings were incorrect, it’s worth noting that I have submitted more OK findings than some reports that were graded "A." Even beyond the initial 15 validated findings, my report continued to contain numerous valid findings.

[L-16] Potential for DoS by returning overly large arrays
[L-20] WithdrawalBatchExpired Event Emitted even if not Processing WithdrawalBatchPayment()
[L-21] Lack of reentrancy modifiers in critical functions depositUpTo(),deposit()
[L-22] collectFees() function not followed the CEI Pattern
[L-23] No Fallback Function for for WildcatSanctionsEscrow contract
[L-25] getAvailableWithdrawalAmount() function potential to Dust Accumulation from Rounding Errors
[L-26] Inefficient Processing of Withdrawal Batches Due to Missing Available Liquidity Check
[L-27] Inability to Remove Deprecated Hook Templates from _hooksTemplates Array
[L-28] Sanctioned Borrowers Allowed to Deploy New Markets, Violating Protocol Compliance Guidelines.

If my understanding of the grading criteria is incorrect, I would appreciate any further clarification.

Thank you for your time and consideration.

[3docSec]

Hi @sathishpic22, no, you are not required to submit only valid findings, far from that.
You are however required to make a net-positive contribution:

In order to be eligible for awards, competitors must contribute more value than they take. This is measured by impact rather than intent.

In other words, the existence of invalid findings does not weigh on this report as much as it does their count relative to the solid findings.
In this context, openly harmful suggestions contribute more negatively than harmless "OK-ish" ones, and L-15 weights the most, because applying basic due diligence would've been such a small effort as compared to what was clearly put in the report, and yet was not given