Paladin - Warden Pledges contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

the function recoverERC20 is very privileged. It means to recover any token that is accidently sent to the contract.

function recoverERC20(address token) external onlyOwner returns(bool) {
	if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();

	uint256 amount = IERC20(token).balanceOf(address(this));
	if(amount == 0) revert Errors.NullValue();
	IERC20(token).safeTransfer(owner(), amount);

	return true;
}

However, admin / owner can use this function to transfer all the reserved reward token, which result in fund loss of the pledge creator and the lose of reward for users that want to delegate the veToken.

Also, the recovered token is sent to owner directly instead of sending to a recipient address.

The safeguard

if(minAmountRewardToken[token] != 0)

cannot stop owner transferring fund because if the owner is compromised or misbehave, he can adjust the whitelist easily.

Proof of Concept

The admin can set minAmountRewardToken[token] to 0 first by calling updateRewardToken

function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {

By doing this the admin remove the token from the whitelist,

then the token can call recoverERC20 to transfer all the token into the owner wallet.

function recoverERC20(address token) external onlyOwner returns(bool) {

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project use multisig wallet to safeguard the owner wallet.

We can also keep track of the reserved amount for rewarding token and only transfer the remaining amount of token out.

 pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;
 reservedReward[token] += totalRewardAmount;

Then we can change the implementation to:

function recoverERC20(address token, address recipient) external onlyOwner returns(bool) {

	uint256 amount = IERC20(token).balanceOf(address(this));
	if(amount == 0) revert Errors.NullValue();

	if(minAmountRewardToken[token] == 0) {
	 // if it is not whitelisted, we assume it is mistakenly sent, 
	   // we transfer the token to recipient
	 IERC20(token).safeTransfer(recipient, amount);
	} else {
	// revert if the owner over transfer
	if(amount >  reservedReward[token]) revert rewardReserved();
	  IERC20(token).safeTransfer(recipient, amount - reservedReward[token]);
	}

	return true;

}

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L438 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L445 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L394 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L401 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L333 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L340

Vulnerability details

Impact

The owner can whitelist any token of ERC20 token as long as they support transferFrom,

but in the current implemnetation, the fee-on-transfer token is not supported

because the totalRewardAmount account is not properly tracked. if we use fee-on-transfer token.

When creating the pledge, the reserved amount of reward token is transferred to the contract.

// Pull all the rewards in this contract
IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(rewardToken).safeTransferFrom(creator, chestAddress, vars.feeAmount);

vars.newPledgeID = pledgesIndex();

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

The contract use vars.totalRewardAmount to transfer and then assume the contract received the extract vars.totalRewardAmount

pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;

In fee-on-transfer token, the received token amount may be less than the transfer amount.

Same issues applies to the logic when the creator wants to update the configuration of the pledge section when creator increasePledgeRewardPerVote or extendPledge

// Pull all the rewards in this contract
IERC20(pledgeParams.rewardToken).safeTransferFrom(creator, address(this), totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(pledgeParams.rewardToken).safeTransferFrom(creator, chestAddress, feeAmount);

// Update the Pledge parameters in storage
pledgeParams.rewardPerVote = newRewardPerVote;

pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;

Proof of Concept

Some tokens take a transfer fee (e.g. STA, PAXG)

Let us use STA as example, it charge 1% of the transfer fee.

If we use STA as a reward token.

the totalRewardAmount calculated is 10000.

// Pull all the rewards in this contract
IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);

but because the fee is charged, we actually receive 9900 amount of token.

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

but we assume we receive 10000 of the token and update pledgeAvailableRewardAmounts.

The contract state means that we have 10000 token reward reserved.

but actually we only have 9900 token reward reserved. This can affect user pledge delegation because we just do not have enough reserved reward token for them.

Tools Used

Manual Review

Recommended Mitigation Steps

we recommend the project check the actual token balance received and then update the pledgeAvailableRewardAmounts map

// Pull all the rewards in this contract

uint256 balanceBefore = IERC20(rewardToken).balanceOf(address(this));

IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(rewardToken).safeTransferFrom(creator, chestAddress, vars.feeAmount);

vars.newPledgeID = pledgesIndex();

uint256 balanceAfter = IERC20(rewardToken).balanceOf(address(this));
uint256 received = balanceAfter - balanceBefore;

vars.totalRewardAmount = received;

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