code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

If an account has a large cooldown, that account can grief other accounts that are waiting for their own cooldowns, by sending small amounts to them.

Proof of Concept

Every transfer to an account increases the cooldown

    /** @dev Hook called before any transfer */
    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        if(from != address(0)) { //check must be skipped on minting
            // Only allow the balance that is unlocked to be transfered
            require(amount <= _availableBalanceOf(from), "hPAL: Available balance too low");
        }

        // Update user rewards before any change on their balance (staked and locked)
        _updateUserRewards(from);

        uint256 fromCooldown = cooldowns[from]; //If from is address 0x00...0, cooldown is always 0 

        if(from != to) {
            // Update user rewards before any change on their balance (staked and locked)
            _updateUserRewards(to);
            // => we don't want a self-transfer to double count new claimable rewards
            // + no need to update the cooldown on a self-transfer

            uint256 previousToBalance = balanceOf(to);
            cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, previousToBalance);
        }

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

The amount of the increase is proportional to the sender's cooldown:

        // Default new cooldown, weighted average based on the amount and the previous balance
        return ((amount * _senderCooldown) + (receiverBalance * receiverCooldown)) / (amount + receiverBalance);

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

Tools Used

Code inspection

Recommended Mitigation Steps

Only allow a total of one cooldown increase when the sender is not the recipient

Kogaroshi commented 2 years ago

As explained in the documentation & the comments for this method, this is required to prevent users to game the system and unstake by skipping the cooldown period. As stated in another Issue of the same kind, this type of behavior, to have an impact on another user cooldown, would require to send an amount of token consequent compared to the receiver balance, acting as a "financial safeguard" against this type of scenario being used frequently.

For another example of this logic, see the stkAAVE system, using the same logic and the same cooldown imapct calculation on transfers

0xean commented 2 years ago

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 am going to side with the warden here. I do see the sponsors argument that this attack is expensive to execute, but is certainly feasible. I think this qualifies as a hypothetical attack path with stated assumptions, but external requirements. The external requirements being someone with enough malice to waste their own money to do so.

While there may not be an easy solution to solve this, it's still a valid risk to raise and for the sponsors to (potentially) disclose to users if there is in fact no way to mitigate it without undesired side effects.

code-423n4 / 2022-03-paladin-findings