QA Report - Githubissues

	Title	Instances
[L-01]	Typo/grammar mistakes	14
[L-02]	Redundant return value	1
[L-03]	Incomplete documentation	13
[L-04]	Wrong usage of NatSpec format comments	24
[L-05]	Missing zero address checks in constructor	1
[N-01]	A constant should be used instead of a magic value	1
[N-02]	Documentation on obvious functions may be redundant	2
[N-03]	Use native time unit rather than defining constants	1
[N-04]	Events can use `indexed` fields for easier querying	1
[N-05]	The `nonReentrant` modifier should occur before all other modifiers	7

[L-01] Typo/grammar mistakes

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L71

protocalFeeRatio should be protocolFeeRatio

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L545

InequalArraySizes should be UnequalArraySizes

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L42

canceled should be cancelled

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L292

taget of votes should be target of votes
balacne should be balance

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L650

Address tof the EC2O token should be Address of the ERC2O token (two mistakes: tof and EC20)

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L295 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L296 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L365 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L366 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L413

ot be pulled should be to be pulled

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L453 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L485

fo should be of

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L339

reards should be rewards

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L621

Platform should not be uppercase when referring to self.
- This is a bit nitpicking, I apologize if I misunderstand its meaning.

[L-02] Redundant return value

Function recoverERC20() uses OpenZeppelin's safeTransfer(), which is guaranteed to revert if the transfer fails. Hence, this function will either revert, or return true, making the return value redundant.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L653-L661

[L-03] Incomplete documentation

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L84-L119

All of the events (13 in total) have incomplete documentation

[L-04] Wrong usage of NatSpec format comments

In general, @notice is meant to be a brief description of the function (i.e. what it's supposed to do, in one line), while @dev is meant to provide devs with clarifications, or some notes on function behavior that one might not expect, or anything noteworthy in general.

The report below points out all found instances of tags misusage, along with recommended mitigation. There are four distinct issues in total listed:

All instances below lists two lines, where the @dev-tagged line and the @notice-tagged line are completely the same. Recommendation: remove the @dev line.

All instances below lists two lines, where the @dev-tagged line is just the @notice line with extra details. Recommendation: Remove the current @notice and replace @dev with @notice.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L361-L362 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L407-L408 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L451-L452 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L483-L484

All instances below lists one line, where the @dev tag is used where @notice should be. Recommendation: Replace @dev with @notice.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L126 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L177 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L216 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L521

All instances below lists one line, where the comment is descriptive enough to be placed in the preceeding @notice tag, or be its own @dev tag. Recommendation: Either combine them with the preceeding @notice tag, or tag it with @dev.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L55 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L66

[L-05] Missing zero address checks in constructor

Specifically, updateChest() has a zero-address sanity check for chestAddress, however there is no such check in the constructor.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L131-L143

[N-01] A constant should be used instead of a magic value

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L626

This is also in line with how the contract defines constants for e.g. MAX_PCT.

[N-02] Documentation on obvious functions may be redundant

I think pause() functions are completely clear and do not need documentation. Furthermore they are admin functions.

However this is an opinion-based non-risk finding, the decision should be ultimately up to the sponsors' preference.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L634 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L641

[N-03] Use native time unit rather than defining constants

The line below can be changed to 7 weeks

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L24

This is also for consistency with how minDelegationTime is defined, as shown below

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L79

[N-04] Events can use `indexed` fields for easier querying

Generally it is a good idea to place indexed fields for database-like data, where it is expected that such data will be queried.

Specifically, the event NewPledge would be extremely helpful to contain indexed fields for creator and receiver, where such values can be expected to be queried by user when needed (e.g. when a receiver wants to query who has pledged them or otherwise).

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L85-L92

[N-05] The `nonReentrant` modifier should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers. Applies to all functions with this modifier.

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L195 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L206 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L307 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L373 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L419 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L456 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L488

code-423n4 / 2022-10-paladin-findings

QA Report #23

[L-01] Typo/grammar mistakes

[L-02] Redundant return value

[L-03] Incomplete documentation

[L-04] Wrong usage of NatSpec format comments

[L-05] Missing zero address checks in constructor

[N-01] A constant should be used instead of a magic value

[N-02] Documentation on obvious functions may be redundant

[N-03] Use native time unit rather than defining constants

[N-04] Events can use `indexed` fields for easier querying

[N-05] The `nonReentrant` modifier should occur before all other modifiers

code-423n4 / 2022-10-paladin-findings

QA Report #23

[L-01] Typo/grammar mistakes

[L-02] Redundant return value

[L-03] Incomplete documentation

[L-04] Wrong usage of NatSpec format comments

[L-05] Missing zero address checks in constructor

[N-01] A constant should be used instead of a magic value

[N-02] Documentation on obvious functions may be redundant

[N-03] Use native time unit rather than defining constants

[N-04] Events can use indexed fields for easier querying

[N-05] The nonReentrant modifier should occur before all other modifiers

[N-04] Events can use `indexed` fields for easier querying

[N-05] The `nonReentrant` modifier should occur before all other modifiers