Expired locks will still get rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L786

Vulnerability details

Impact

Currently contract has no check to verify if the locked amount is already expired while calculating rewards. This means users can get reward even when there locked duration has expired

Proof of Concept

1. User locks amount 100 for duration 20 days
2. 25 days have gone and User lock is expired. No one has still called kick function
3. User claims the rewards using claim function.
4. Since there is no check to see whether amount 100 is already expired so User obtains higher rewards for full 100 amount even though lock duration is already expired

Recommended Mitigation Steps

Add a check:

vars.lastUserLockIndex = userLocks[user].length - 1;
        UserLock storage currentUserLock = userLocks[user][vars.lastUserLockIndex];
        uint256 userCurrentLockEnd = currentUserLock.startTimestamp + currentUserLock.duration;

        require(block.timestamp > userCurrentLockEnd, "hPAL: Not expired");

[Kogaroshi]

In that scenario, there is 1 major point of the system that is not considered: the bonusRatio of the user is decreasing over the Lock duration, to reach a x1 multiplier on the rewards at Lock expiry. But as said in the doc and the comments, if the Lock is expired, then the BonusRatio will continue to decrease under the x1 multiplier, down to 0. This means than users which Lock expired will get a malus over the rewards, and the locked balance will effectively accrue less rewards than a simply staked balance.

[0xean]

Agree with the sponsor based on the code and clarification they added re: the bonusRatio