Paladin contest - robee's results

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:
        
        Instead a < b / c use a * c < b. 
    
In all of the big and trusted contracts this rule is maintained.

    HolyPaladinToken.sol, 633, uint256 bonusVotes = delegates[user] == user && userLocks[user][nbLocks - 1].duration >= ONE_YEAR ? (lockAmount * bonusLockVoteRatio) / UNIT : 0; 
    HolyPaladinToken.sol, 758, uint256 ratio = currentTotalSupply > 0 ? (accruedBaseAmount * UNIT) / currentTotalSupply : 0; 
    HolyPaladinToken.sol, 652, uint256 bonusVotes = getPastDelegate(user, blockNumber) == user && pastLock.duration >= ONE_YEAR ? (pastLock.amount * bonusLockVoteRatio) / UNIT : 0; 

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    HolyPaladinToken.emergencyWithdraw
    HolyPaladinToken.triggerEmergencyWithdraw

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    ERC20.sol._approve owner
    ERC20.sol.allowance owner
    Ownable.sol.transferOwnership newOwner
    Ownable.sol._setOwner newOwner

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    HolyPaladinToken.sol.allBalancesOf user
    HolyPaladinToken.sol._moveDelegates from
    ERC20.sol._afterTokenTransfer from
    HolyPaladinToken.sol._getNewReceiverCooldown receiver
    HolyPaladinToken.sol._stake user
    ERC20.sol.decreaseAllowance spender

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in PaladinRewardReserve.sol line 37: IERC20(token).safeApprove(spender, 0);

Deprecated safeApprove in SafeERC20.sol line 64: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));

Deprecated safeApprove in SafeERC20.sol line 55: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));

Deprecated safeApprove in PaladinRewardReserve.sol line 46: IERC20(token).safeApprove(spender, 0);

Deprecated safeApprove in PaladinRewardReserve.sol line 30: IERC20(token).safeApprove(spender, amount);

Deprecated safeApprove in PaladinRewardReserve.sol line 38: IERC20(token).safeApprove(spender, amount);

Deprecated safeApprove in SafeERC20.sol line 76: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    Ownable.sol
    AccessControl.sol

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: HolyPaladinToken.sol, In line 1236 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 183 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 182 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 1138 with Empty Require message.

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    HolyPaladinToken.sol, _getUserAccruedRewards

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    PaladinRewardReserve.sol, updateSpenderAllowance is missing a reentrancy modifier
    PaladinRewardReserve.sol, removeSpender is missing a reentrancy modifier
    PaladinRewardReserve.sol, setNewSpender is missing a reentrancy modifier

Title: Must approve 0 first Severity: Low/Med Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first PaladinRewardReserve.sol, 30, IERC20(token).safeApprove(spender, amount);

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    Math.sol (L40) a might be 0)
    Math.sol (L40) b might be 0)
    HolyPaladinToken.sol (L1131) receiver might be 0)
    HolyPaladinToken.sol (L1131) receiverBalance might be 0)
    HolyPaladinToken.sol (L1131) amount might be 0)

Title: Override function but with different argument location Severity: Low/Med Risk

    HolyPaladinToken.sol.constructor inherent Ownable.sol.constructor but the parameters does not match
    PaladinRewardReserve.sol.constructor inherent Ownable.sol.constructor but the parameters does not match
    PaladinRewardReserve.sol.constructor inherent ReentrancyGuard.sol.constructor but the parameters does not match
    HolyPaladinToken.sol.constructor inherent ERC20.sol.constructor but the parameters does not match

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    MerkleProof.sol, proof, 28

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: MerkleProof.sol, i, 28

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    HolyPaladinToken.sol, _unstake
    AccessControl.sol, _checkRole
    HolyPaladinToken.sol, _kick
    HolyPaladinToken.sol, _unlock
    HolyPaladinToken.sol, _getPastVotes
    HolyPaladinToken.sol, _updateDropPerSecond
    SafeERC20.sol, safeTransfer

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    HolyPaladinToken.sol, 1078: change 'amount > 0' to 'amount != 0'
    HolyPaladinToken.sol, 385: change 'amount > 0' to 'amount != 0'
    HolyPaladinToken.sol, 229: change 'balance > 0' to 'balance != 0'
    ERC20.sol, 280: change 'balance > 0' to 'balance != 0'
    HolyPaladinToken.sol, 1062: change 'amount > 0' to 'amount != 0'
    HolyPaladinToken.sol, 1026: change 'amount > 0' to 'amount != 0'
    ERC20.sol, 231: change 'balance > 0' to 'balance != 0'
    HolyPaladinToken.sol, 1342: change 'amount > 0' to 'amount != 0'
    Address.sol, 55: change 'balance > 0' to 'balance != 0'
    Address.sol, 128: change 'balance > 0' to 'balance != 0'
    HolyPaladinToken.sol, 800: change 'balance > 0' to 'balance != 0'

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    SafeERC20.sol, safeTransferFrom
    Context.sol, _msgData
    AccessControl.sol, _setRoleAdmin
    Address.sol, sendValue
    ERC20.sol, _mint
    ECDSA.sol, toTypedDataHash
    HolyPaladinToken.sol, _getPastDelegate
    SafeERC20.sol, safeApprove
    Strings.sol, toString
    Math.sol, ceilDiv
    ERC20.sol, _burn
    Math.sol, max
    ECDSA.sol, toEthSignedMessageHash
    Math.sol, average
    HolyPaladinToken.sol, _afterTokenTransfer
    SafeERC20.sol, safeDecreaseAllowance
    AccessControl.sol, _setupRole
    Context.sol, _msgSender
    SafeERC20.sol, safeIncreaseAllowance
    MerkleProof.sol, verify
    Math.sol, min

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    SafeERC20.sol, safeTransferFrom
    Strings.sol, toHexString
    HolyPaladinToken.sol, _beforeTokenTransfer
    Context.sol, _msgData
    HolyPaladinToken.sol, _getUserAccruedRewards
    AccessControl.sol, _setRoleAdmin
    Address.sol, verifyCallResult
    ERC20.sol, _beforeTokenTransfer
    ERC20.sol, _transfer
    ERC20.sol, _afterTokenTransfer
    Address.sol, sendValue
    Address.sol, functionCallWithValue
    Address.sol, isContract
    HolyPaladinToken.sol, safe224
    ERC20.sol, _mint
    Math.sol, min
    HolyPaladinToken.sol, _unlock
    HolyPaladinToken.sol, _lock
    HolyPaladinToken.sol, _kick
    ECDSA.sol, toTypedDataHash
    HolyPaladinToken.sol, _writeCheckpoint
    HolyPaladinToken.sol, _updateDropPerSecond
    Address.sol, functionStaticCall
    ECDSA.sol, tryRecover
    HolyPaladinToken.sol, safe128
    HolyPaladinToken.sol, _getPastDelegate
    SafeERC20.sol, safeApprove
    SafeERC20.sol, safeTransfer
    HolyPaladinToken.sol, _getNewReceiverCooldown
    HolyPaladinToken.sol, _availableBalanceOf
    Strings.sol, toString
    AccessControl.sol, _checkRole
    HolyPaladinToken.sol, _updateRewardState
    Math.sol, ceilDiv
    ERC20.sol, _burn
    HolyPaladinToken.sol, _getNewIndex
    ERC20.sol, _approve
    ECDSA.sol, toEthSignedMessageHash
    Math.sol, max
    Math.sol, average
    HolyPaladinToken.sol, _getPastLock
    HolyPaladinToken.sol, _afterTokenTransfer
    HolyPaladinToken.sol, _unstake
    HolyPaladinToken.sol, _delegate
    HolyPaladinToken.sol, _stake
    HolyPaladinToken.sol, safe48
    HolyPaladinToken.sol, _updateUserRewards
    HolyPaladinToken.sol, safe32
    ECDSA.sol, recover
    HolyPaladinToken.sol, _getPastVotes
    HolyPaladinToken.sol, _moveDelegates
    AccessControl.sol, _setupRole
    Context.sol, _msgSender
    SafeERC20.sol, safeIncreaseAllowance
    Address.sol, functionDelegateCall
    MerkleProof.sol, verify
    SafeERC20.sol, safeDecreaseAllowance
    Address.sol, functionCall

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    _name in ERC20.sol
    _symbol in ERC20.sol

Title: Change if -> revert pattern to require Severity: GAS

Change if -> revert pattern to 'require' to save gas and improve code quality, if (some_condition) { revert(revert_message) }

to: require(!some_condition, revert_message)

In the following locations:

    AccessControl.sol, 96

Title: Storage double reading. Could save SLOAD Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    HolyPaladinToken.sol: currentDropPerSecond is read twice in _updateDropPerSecond
    HolyPaladinToken.sol: minLockBonusRatio is read twice in _lock

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    HolyPaladinToken.sol (L#1119) - uint256 minValidCooldown = block.timestamp - (COOLDOWN_PERIOD + UNSTAKE_PERIOD); 
    HolyPaladinToken.sol (L#1083) - require(block.timestamp > (userCooldown + COOLDOWN_PERIOD), "hPAL: Insufficient cooldown");
    HolyPaladinToken.sol (L#1084) - require(block.timestamp - (userCooldown + COOLDOWN_PERIOD) <= UNSTAKE_PERIOD, "hPAL: unstake period expired"); 
    HolyPaladinToken.sol (L#727) - if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; 
    HolyPaladinToken.sol (L#1283) - require(block.timestamp > userCurrentLockEnd + UNLOCK_DELAY, "hPAL: Not kickable"); 
    HolyPaladinToken.sol (L#1434) - if(block.timestamp < startDropTimestamp + dropDecreaseDuration) revert DecreaseDurationNotOver();
    HolyPaladinToken.sol (L#716) - if(block.timestamp > startDropTimestamp + dropDecreaseDuration) { 

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: ERC20.sol, In line 226, Require message length to shorten: 35, The message: ERC20: transfer to the zero address
    Solidity file: ERC20.sol, In line 310, Require message length to shorten: 34, The message: ERC20: approve to the zero address
    Solidity file: ERC20.sol, In line 225, Require message length to shorten: 37, The message: ERC20: transfer from the zero address
    Solidity file: Address.sol, In line 183, Require message length to shorten: 38, The message: Address: delegate call to non-contract
    Solidity file: ERC20.sol, In line 198, Require message length to shorten: 37, The message: ERC20: decreased allowance below zero
    Solidity file: ERC20.sol, In line 275, Require message length to shorten: 33, The message: ERC20: burn from the zero address
    Solidity file: Address.sol, In line 156, Require message length to shorten: 36, The message: Address: static call to non-contract
    Solidity file: ERC20.sol, In line 157, Require message length to shorten: 40, The message: ERC20: transfer amount exceeds allowance
    Solidity file: Ownable.sol, In line 62, Require message length to shorten: 38, The message: Ownable: new owner is the zero address
    Solidity file: ERC20.sol, In line 280, Require message length to shorten: 34, The message: ERC20: burn amount exceeds balance
    Solidity file: Address.sol, In line 128, Require message length to shorten: 38, The message: Address: insufficient balance for call
    Solidity file: ERC20.sol, In line 231, Require message length to shorten: 38, The message: ERC20: transfer amount exceeds balance
    Solidity file: ERC20.sol, In line 309, Require message length to shorten: 36, The message: ERC20: approve from the zero address

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    MerkleProof.sol, 28

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    PaladinRewardReserve.sol; the inherited contracts ReentrancyGuard not used
    AccessControl.sol; the inherited contracts ERC165 not used

Title: Unnecessary Reentrancy Guards Severity: GAS

Where there is onlyOwner or Initializer modifer, the reentrancy gaurd isn't necessary (unless you don't trust the owner or the deployer, which will lead to full security breakdown of the project and we believe this is not the case) This is a list we found of such occurrences:

    PaladinRewardReserve.sol no need both nonReentrant and onlyOwner modifiers in transferToken

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    ERC20.constructor (symbol_)
    Address.functionCall (data)
    Address.functionCallWithValue (data)
    Address.functionCall (errorMessage)
    Address.verifyCallResult (errorMessage)
    ERC20.constructor (name_)
    Address.functionCallWithValue (errorMessage)
    Address.functionStaticCall (errorMessage)
    Address.functionDelegateCall (errorMessage)

Title: Unnecessary default assignment Severity: GAS

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

    HolyPaladinToken.sol (L#103) : bool public emergency = false; 
    HolyPaladinToken.sol (L#100) : uint256 public bonusLockVoteRatio = 0.5e18; 

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    AccessControl.sol, revokeRole
    AccessControl.sol, renounceRole
    ERC20.sol, symbol
    ERC20.sol, name
    AccessControl.sol, supportsInterface
    ERC20.sol, transfer
    ERC20.sol, approve
    AccessControl.sol, hasRole
    ERC20.sol, allowance
    ERC20.sol, balanceOf
    ERC20.sol, transferFrom
    ERC20.sol, decreaseAllowance
    ERC20.sol, totalSupply
    Ownable.sol, renounceOwnership
    ERC20.sol, decimals
    Ownable.sol, owner
    AccessControl.sol, grantRole
    ERC20.sol, increaseAllowance
    Ownable.sol, transferOwnership

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    Context.sol, _msgData, { return msg.data; }
    Context.sol, _msgSender, { return msg.sender; }
    Math.sol, min, { return a < b ? a : b; }
    Math.sol, max, { return a >= b ? a : b; }
    Address.sol, functionCall, { return functionCallWithValue(target, data, 0, errorMessage); }
    ECDSA.sol, toTypedDataHash, { return keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); }