Cyfrin / 2023-07-escrow

17 stars 12 forks source link

Add a clear NatSpec comment. #655

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Add a clear NatSpec comment.

Severity

Gas Optimization / Informational

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/IEscrow.sol#L41C1-L43C58

Summary

Comments should be clear and the functionality of the function and it's parameter should be clearly mentioned.

Vulnerability Details

In the IEscrow.sol contract, the code comment of resolveDispute function is rather ambiguous and the function parameter and it's functionality is not documented properly.

Impact

It creates ambiguity and misinterpretation for the function and it's functionality.

Tools Used

Manual Analysis

Recommendations

Add a clear NatSpec comment for the function and it's parameters which clearly explains the functionality of the function.

PatrickAlphaC commented 1 year ago

Need to give the recommendation of what you think "clear" looks like

0xSandyy commented 1 year ago

Escalate Dupe of #443. I should have been clear with the suggestion but I thought this issue is pretty self-explanatory and may not need much of a suggestion. But, my version of clear NatSpec would be like this:

    /**
     * @notice Arbiter can resolve dispute and specify buyerAward(can be 0) but `buyerAward` plus `arbiterFee`(set at construction) can't be more than `price`(Total token balance of this contract).
     * @param buyerAward The amount of tokens to be awarded to the buyer if dispute is resolved in favor of the buyer.
     */
PatrickAlphaC commented 1 year ago

That one listed all the areas where this needs to happen. You linked to a line that already has natspec.

/// @notice Arbiter can resolve dispute and claim token reward by entering in split of `price` value,

/// minus arbiterFee set at construction. function resolveDispute(uint256 buyerAward) external;