Upgraded Q -> 3 from #23 [1722887783819]

[c4-judge]

Judge has assessed an item in Issue #23 as 3 risk. The relevant finding follows:

[L-04] LandManager::transferToUnoccupiedPlot() function doesn't update the toilerState.dirty mapping variable

The transferToUnoccupiedPlot() function allows users to transfer their NFTs to an unoccupied slot, however only the latestTaxRate is updated, which is not necessary as the latestTaxRate is updated in the _farmPlots() function before the logic in the transferToUnoccupiedPlot() function which is called beforehand. However the dirty variable in the mapping(uint256 => ToilerState) public toilerState; mapping is not updated

    struct ToilerState {
        uint256 lastToilDate;
        uint256 plotId;
        address landlord;
        uint256 latestTaxRate;
        bool dirty;
    }

This will be a problem and result in a user loosing rewards in the following scenario. Consider that a user stakes his NFT in the last available plot for a certain landlord, however the landlord decreases his lock in the LockManager contract, and now has less available plots. When the user that staked in the last plot tries to farm his accrued rewards and the farmPlots() function is called which in turn calls the _farmPlots() internal function, the dirty variable will be set to true. If a user sees that the available plots of a landlord have decreased and directly calls the transferToUnoccupiedPlot() function, the _farmPlots() internal function logic will be executed before the logic in transferToUnoccupiedPlot() and the dirty variable will be set to true again. If a user transfers his NFT to a previous plot, that should be receiving rewards the dirty variable will still be equal to true, and the user won't be able to accrue and later farm any rewards.

Consider the following example:

• LandLordA has 10 available plots, userA stakes his NFT at plotId 9.
• LandLordA decreases his lock and now only have 5 available plots.
• UserA sees that and decides to move his NFT to plotId 4, which is not occupied, in order to continue receiving rewards. However when he calls the transferToUnoccupiedPlot() function, it has the forceFarmPlots() modifier which will first execute the logic in the _farmPlots() function before the logic in the transferToUnoccupiedPlot() function, which will set the dirty variable to true.

          if (_getNumPlots(landlord) < _toiler.plotId) {
              timestamp = plotMetadata[landlord].lastUpdated;
              toilerState[tokenId].dirty = true;
          }

  Now plotId 4 will be occupied in the mapping(address => mapping(uint256 => Plot)) plotOccupied;, but the dirty variable will still be set to true. The next time the user tries to farm his accrued rewards, he won't receive any rewards, the landlord also won't receive the tax rewards he is entitled to. The plotId in the ToilerState is not updated as well, however this is different vulnerability, as a malicious actor can utilize it to DOS all landlord plots. The user here will be thinking that he is acccruing rewards, but when he tries to farm them he won't receive any(if that is his only stake). This results in losses for rewards for both the landlord and the user who first staked, and then transferred his NFT to a different plotId.

Recommended Mitigation Steps

When the transferToUnoccupiedPlot() function is called set the dirty variable to false, as the function already has logic that checks if the plotId that the user is trying to transfer his NFT, should be receiving rewards or not.

if (plotId >= totalPlotsAvail) revert PlotTooHighError();

[c4-judge]

alex-ppg marked the issue as duplicate of #26

[c4-judge]

alex-ppg marked the issue as partial-75

[AtanasDimulski]

Hey @alex-ppg, thanks for the swift judging. This issue describes the exact scenario for the attack outlined in #26, and provides the same fix. While I may argue that this report is better than the primary one,( I have even provided a step-by-step explanation of the attack) I understand that selecting the best report is a subjective task. However, I don't agree that this report deserves a partial-75 credit. As you can see the report isn't similar to other low findings, and there is a reason for that - I thought the impact was not high, as the user could simply stake and usntake, but expected that it could be regarded as an H/M issue. I urge you to give this report full credit. Thanks for your time!

[alex-ppg]

Hey @AtanasDimulski, thank you for your PJQA contribution. A penalty of 25% was applied as the report was submitted as part of a QA report incorrectly, and it is not unexpected to consider its HM severity given that the function is useless without the cleaning of the dirty flag.

A second 25% penalty has been applied after discussions on #281, rendering this submission's total reward as partial-50.

[c4-judge]

alex-ppg marked the issue as partial-50

[AtanasDimulski]

Hey @alex-ppg , thanks for completely disrespecting my work, can you please point me to the section in the comprehensive docs, where it is mentioned that submitting a finding as a QA report, renders a penalty of 25%, or this applies only for this contest. Also given the fact that every judge judges in his completely own way, it is not always the case where such finding can be accepted as an H/M. Also given this: The requisites of a full mark report are:

Identification and demonstration of the root cause

Identification and demonstration of the maximum achievable impact of the root cause

I don't see how another 25% cut is appropriate. Thanks for your time!

[alex-ppg]

Hey @AtanasDimulski, thank you for your contribution. I would like to note that PJQA has concluded and no further feedback is expected on any submission.

I understand that the penalization may be frustrating, however, assessing the correct severity level of a vulnerability is the responsibility of the Wardens. A vulnerability that has a QA (L) risk can be "safely acknowledged" by Sponsors, and if the Sponsor proceeded with such an action on this one it would have significant consequences. As such, a penalization has been applied due to misjudging the severity of the vulnerability (i.e. not properly understanding the maximum achievable impact in terms of severity).

A further penalization of 25% has been applied per the discussions on #281 and in line with the C4 guidelines.

[AtanasDimulski]

Hey @alex-ppg, thanks for your swift reply. However, I couldn't find the reason you mentioned specified in the docs. So there are two options, either I missed it, I would like you to point me to the specific section in the docs where this is mentioned. Or for some unknown reason, you have something personal against me, and you are just trying to cut me with a made-up argument. I sincerely hope It is the first. Thanks for your time!

[alex-ppg]

Hey @AtanasDimulski, thank you for your feedback. Insinuating that there is some personal factor when I have applied the same practice (penalization of QA report upgrades) over 13 audits is ill-intended, and you have not supplied any meaningful contribution to the discussion which goes against the good citizen policy.

The C4 documentation and specifically the SC Verdicts demonstrate that lack of maximal impact identification is grounds for penalization, and submitting a high-risk submission as low-risk falls under that category as I mentioned earlier.

I have already specified why submitting the issue as part of a QA report that is unlikely to be considered seriously by the Sponsor merits a penalization, and have highlighted documentation that this type of penalization would fall under. I will discuss with fellow judges to formalize this in a more concrete entry to the C4 documentation and you are more than welcome to discuss this particular point over a C4 organization issue, however, this is not the appropriate avenue to continue this discussion.

[AtanasDimulski]

I believe I have provided a meaningful contribution to the discussion. What you are referring to is not identifying the maximum impact, and in my understanding, this has nothing to do with the severity assigned to the issue. I have provided the following impact: The user here will be thinking that he is accruing rewards, but when he tries to farm them he won't receive any(if that is his only stake). This results in losses for rewards for both the landlord and the user who first staked, and then transferred his NFT to a different plotId., which I believe is the maximum impact. As for whether the sponsors considers issues to be serious or not, there is nothing that wardens can do(specifically for this contest, the sponsor has checked all QA reports with grade-a and provided feedback). Maybe I don't understand the difference between severity and impact. Anyway, I don't intend to continue arguing, it is what it is. Thank you for your time!