search

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

2 stars 0 forks source link

[M2] Zero protocol fee can prevent users to extendPledge for some tokens #266

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L335 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L396

Vulnerability details

Impact

Users are unable neither to createPleadge nor extendPledge if protocol Fees are set to zero for some tokens

Proof of Concept

It is not stated in documentation if you allow protocol fees to zero. For your code it is possible to owner to set the fee to zero.

 function updatePlatformFee(uint256 newFee) external onlyOwner {
    if(newFee > 500) revert Errors.InvalidValue();
    uint256 oldfee = protocalFeeRatio;
    protocalFeeRatio = newFee;
    emit PlatformFeeUpdated(oldfee, newFee);
}

However there are many non-malicious tokens that will revert if you try to transfer zero tokens. For these cases the function createPledge and extendPledge will revert when trying to transfer fees to chest

IERC20(rewardToken).safeTransferFrom(creator, chestAddress, vars.feeAmount);  @audit will revert if fee is zero

An scenario is that some user create a pledge, then the owner decides to set fee to zero for some reason. That user will be unable to extend pledge and pledge will expiry.

Recommended

If you do not plan to set fee to zero it is better to set a minimum fee.

trust1995 commented 2 years ago

Good suggestion. I don't think protocol fee is ever intended to be 0. Since the disrupted activity does not risk any funds, and can be restored quickly by the team, added to the low likelihood of fee being 0, it seems to be low severity issue.

Kogaroshi commented 2 years ago

Fixed in PR 2, commit Better be safe than breaking anything. But disagree with severity, this should not be Medium

kirk-baird commented 1 year ago

I'm downgrading this to QA. This case should not rise and if it does there is a simple fix such that no funds are lost.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

kirk-baird marked the issue as grade-b