code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The accounting for the reward token amount currently assumes that the WardenPledge contract will have a balance equal to an amount x if a transferFrom for said amount x happens to the contract. If an ERC20 token gets whitelisted that has a fee-on-transfer mechanism and is subsequently used for a pledge, the accounting will be incorrect and the creator of the pledge will not be able to close the pledge or withdraw the remaining tokens.

Proof of Concept

In the function createPledge the amount that is assigned to pledgeAvailableRewardAmounts will be the same as was was requested in a call to rewardtoken.safeTransferFrom:

IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);
...

vars.newPledgeID = pledgesIndex();

// Add the total reards as available for the Pledge & write Pledge parameters in storage
pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;

In case of rewardToken having fees-on-transfer, the actual value by which the balance of reward tokens has increased on the WardenPledge contract will be lower than that. This can make the functions retrievePledgeRewards and closePledge uncallable if there is only one currently running pledge with that token as a reward, as they both try to perform to transfer more tokens than the contract currently holds:

// Get the current remaining amount of rewards not distributed for the Pledge
uint256 remainingAmount = pledgeAvailableRewardAmounts[pledgeId];

if(remainingAmount > 0) {
    // Transfer the non used rewards and reset storage
    pledgeAvailableRewardAmounts[pledgeId] = 0;

    IERC20(pledgeParams.rewardToken).safeTransfer(receiver, remainingAmount);

In case of multiple running pledges the first pledge creators to call either function will effectively steal tokens from other pledges.

Tools Used

Manual Review

Recommended Mitigation Steps

Option 1: Track the value of the balanceOf function of the reward token before and after a safeTransferFrom and assign the difference to pledgeAvailableRewardAmounts[vars.newPledgeID]
Option 2: ensure through some (offchain-)means that no fee-on-transfer tokens will ever get whitelisted, if the system is not meant to be compliant with them.

Kogaroshi commented 1 year ago

Duplicate of #27

c4-judge commented 1 year ago

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