Paladin contest - gzeon's results

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1131

Vulnerability details

Impact

Due to how the cooldown period is calculated after a transfer, a user can strategically transfer between accounts to increase their cooldown timestamp while keeping it within the UNSTAKE_PERIOD, so they can unstake anytime, defeating the cooldown mechanism.

Proof of Concept

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1131

        return ((amount * _senderCooldown) + (receiverBalance * receiverCooldown)) / (amount + receiverBalance);

Pin Solidity Version

Consider to pin Solidity version to latest 0.8.12

Divide before Multiply

HolyPaladinToken._updateDropPerSecond() (contracts/HolyPaladinToken.sol#714-742) performs a multiplication on the result of a division: -dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH) (contracts/HolyPaladinToken.sol#729) -nbMonthEllapsed = (block.timestamp - lastDropUpdate) / MONTH (contracts/HolyPaladinToken.sol#730) -dropPerSecondDecrease = dropDecreasePerMonth * nbMonthEllapsed (contracts/HolyPaladinToken.sol#732) HolyPaladinToken._getNewIndex(uint256) (contracts/HolyPaladinToken.sol#744-761) performs a multiplication on the result of a division: -baseDropPerSecond = (_currentDropPerSecond * UNIT) / maxLockBonusRatio (contracts/HolyPaladinToken.sol#752) -accruedBaseAmount = ellapsedTime * baseDropPerSecond (contracts/HolyPaladinToken.sol#755) HolyPaladinToken._getUserAccruedRewards(address,uint256) (contracts/HolyPaladinToken.sol#786-851) performs a multiplication on the result of a division: -lockingRewards = (userLockedBalance * ((indexDiff * vars.periodBonusRatio) / UNIT)) / UNIT (contracts/HolyPaladinToken.sol#842) HolyPaladinToken._lock(address,uint256,uint256,HolyPaladinToken.LockAction) (contracts/HolyPaladinToken.sol#1137-1233) performs a multiplication on the result of a division: -durationRatio = ((duration - MIN_LOCK_DURATION) * UNIT) / (MAX_LOCK_DURATION - MIN_LOCK_DURATION) (contracts/HolyPaladinToken.sol#1156) -userLockBonusRatio = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio) / UNIT) (contracts/HolyPaladinToken.sol#1157) HolyPaladinToken._lock(address,uint256,uint256,HolyPaladinToken.LockAction) (contracts/HolyPaladinToken.sol#1137-1233) performs a multiplication on the result of a division: -durationRatio_scope_0 = ((duration - MIN_LOCK_DURATION) * UNIT) / (MAX_LOCK_DURATION - MIN_LOCK_DURATION) (contracts/HolyPaladinToken.sol#1212) -userLockBonusRatio_scope_1 = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio_scope_0) / UNIT) (contracts/HolyPaladinToken.sol#1213) HolyPaladinToken._kick(address,address) (contracts/HolyPaladinToken.sol#1270-1315) performs a multiplication on the result of a division: -nbWeeksOverLockTime = (block.timestamp - userCurrentLockEnd) / WEEK (contracts/HolyPaladinToken.sol#1305) -penaltyPercent = nbWeeksOverLockTime * kickRatioPerWeek (contracts/HolyPaladinToken.sol#1306) Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

Used block.timestamp instead of block.number

Same result but inconsistant with comment https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L861

Missing event on critical parameter change

setKickRatio https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1416 setEndDropPerSecond https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1433

Lack input validation on _rewardsVault

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L194

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

contracts/HolyPaladinToken.sol:229:        require(balanceOf(msg.sender) > 0, "hPAL: No balance");
contracts/HolyPaladinToken.sol:385:        require(amount > 0, "hPAL: incorrect amount");
contracts/HolyPaladinToken.sol:1062:        require(amount > 0, "hPAL: Null amount");
contracts/HolyPaladinToken.sol:1078:        require(amount > 0, "hPAL: Null amount");
contracts/HolyPaladinToken.sol:1237:        require(userLocks[user].length > 0, "hPAL: No Lock");
contracts/HolyPaladinToken.sol:1246:        require(currentUserLock.amount > 0, "hPAL: No Lock");
contracts/HolyPaladinToken.sol:1272:        require(userLocks[user].length > 0, "hPAL: No Lock");
contracts/HolyPaladinToken.sol:1281:        require(currentUserLock.amount > 0, "hPAL: No Lock");
contracts/HolyPaladinToken.sol:1342:        require(amount > 0, "hPAL: Null amount");

Unnecessary nonReentrant modifier in transferToken

Since it is onlyOwner, we don't really need to prevent reentrancy https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L52

Use uint256 instead of bool

Use uint(1) instead of bool(true) to save gas by avoiding masking ops https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L52

Unchecked safe math

This can be unchecked as Total locked must be greater than or equal to currentUserLock https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1286

        currentTotalLocked -= currentUserLock.amount;

This can be uncheced as WEEK != 0 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1305

        uint256 nbWeeksOverLockTime = (block.timestamp - userCurrentLockEnd) / WEEK;