code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

By design, a user's cooldown period is extended if they receive a transfer of hPal. The cooldown is extended based on the weight of the receiver's original balance and cooldown period compared to the sent amount and sender's cooldown period. Due to this mechanic, a malicious but generous user can indefinitely freeze the funds of another user by continuously extending the receiver's cooldown period, disabling them from unstaking.

In the most extreme case, it is possible that Pal can have negative price action in the period while the funds are frozen such that the user will be losing value despite accruing more hPal. Disregarding negative price action, having a user's funds frozen based on the authority of others would not be desired.

Proof of Concept

User A stakes 10 Pal and receives 10 hPal
User A calls cooldown() and waits 9 days, making his remaining cooldown period equal 1 day
User B stakes 10 Pal, receives 10 hPal and starts their cooldown()
User B immediately sends all 10 hPal to User A.
User A now has a cooldown period = 5.5 days based on the following formula.

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

With values:

((10 hPal * 10 days) + (10 hPal * 1 day)) / (10 hPal + 10 hPal) = 5.5 days

Tools Used

Manual review

Recommended Mitigation Steps

Understanding that this is a mechanism to combat users transferring funds between their own accounts to work around the cooldown system, I recommend adding an acceptTransfer() function. This function will allow the receiver to accept the sent funds and resulting new cooldown period.

Kogaroshi commented 2 years ago

This behavior is wanted in the token design. Any implementation of a acceptTransfer() or any similar design will remove the ability of the hPAL token to be compatible with the ERC20 design, which is not something desired for that token.

And as shown in the example, to effectively have an impact on the cooldown of another user through a transfer, it would require an important amount of token (100% of the balance to push back to 5 days out of 10 days cooldown if the target cooldown is about to be reached), which is the desired logic to reduce this kind of scenario

(for a live example, this system is taken from the stkAave system, where that type of scenario is rarely seen)

0xean commented 2 years ago

Downgrading to Medium based on the fact that there does an exist an attack vector, albeit an expensive one.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I think this falls into the hypothetical attack path with external requirements and therefore is a Med.