Whole balance self transfer set user's cooldown to zero

[code423n4]

Lines of code

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

Vulnerability details

Impact

If a user transfers to self the whole balance, then user’s cooldown resets to zero.

A user will have to reset its cooldown manually to current timestamp via cooldown() call to be able to unstake in the future.

As a transfer to self can happen by mistake and there are no contract logic level reasons to reset cooldown in this case, it is a temporal user fund freeze kind of issue

Proof of Concept

The cooldown reset happens even if a user self-transfers (from = to):

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

Unstake will be unavailable until cooldown be reset again:

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

Which is done by cooldown() via setting it to the current timestamp, so the user cooldown will be effectively moved to the future, increasing the amount of time user have to keep the funds with the contract without any additional rewards:

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

This effectively means that user funds be frozen for an additional period, which is the difference between his former cooldown and current timestamp. There is no reason for that and it will go against user’s expectations

Recommended Mitigation Steps

Consider excluding self-transfer case from cooldown reset logic, for example:

Now:

if(from != to) {
   ...
   cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, previousToBalance);
}

// If from transfer all of its balance, reset the cooldown to 0
uint256 previousFromBalance = balanceOf(from);
if(previousFromBalance == amount && fromCooldown != 0) {
   cooldowns[from] = 0;
}

To be:

if(from != to) {
   ...
   cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, previousToBalance);

   // If from transfer all of its balance, reset the cooldown to 0
   uint256 previousFromBalance = balanceOf(from);
   if(previousFromBalance == amount && fromCooldown != 0) {
   cooldowns[from] = 0;
   }
}

[Kogaroshi]

Duplicate of https://github.com/code-423n4/2022-03-paladin-findings/issues/8 See in the Issue #8 for PR with changes

[0xean]

closing as dupe of #8