VaultDelegate::sendReward() - Token fees not subtracted from vault balance before rewards are sent, triggering DoS of reward distribution functionality.
Smart contract unable to operate due to lack of token funds
Temporary freezing of funds
Contract fails to deliver promised returns, but doesn't lose value
Description
Brief/Intro
(This bug report focuses on the native token section of the affected function, but the bug appears to be present in the ERC20 token section too, in the affected function. So this report + bugfix should apply to the ERC20 section too with the appropriate modifications of course.)
The existing protocol test RewardTimelock.t.sol::testQueuesAndExecutesRewardTx() could not/did not catch this bug because of the following reasons:
total of 1.1 ether was deposited into the vault via vm.deal() cheat-code function, but only 1 ether was queued during queued transaction for the native token amount, instead of the full 1.1 ether. And since the native token fee worked out to 0.1 ether exactly, the bug was not triggered, and the reward distribution transaction was successful with zero balance left in the vault afterwards.
Vulnerability Details
What's the bug exactly?
So the bug is that the sendReward() function does not subtract the fee from the vault balance, and then uses the same vault balance to send the reward to the whitehat. Simple example:
If the total vault balance is 100, and the fee is 10, then after the fee was sent the reward amount to be sent is still 100, instead of 90.
But the existing protocol test missed this as per previous explanation above.
My PoC down below demonstrates how the reward is actually successfully sent WITH the bugfix, but the transaction fails WITHOUT the bugfix.
This is where the native token fee amount is calculated: uint256 nativeTokenFee = (nativeTokenAmount * feeBps) / feeBasis; And this is where the same nativeTokenAmount as above is used to send the reward to the whitehat: (bool success, ) = to.call{ value: nativeTokenAmount, gas: gasToTarget }(""); The nativeTokenFee should have been subtracted from the nativeTokenAmount first, but it wasn't.
The buggy function:
functionsendReward(uint96 referenceId,address to,Rewards.ERC20Reward[] calldata tokenAmounts,uint256 nativeTokenAmount,uint256 gasToTarget ) external {require(gasToTarget <= UNTRUSTED_TARGET_GAS_CAP,"VaultDelegate: gasToTarget greater than max allowed"); (uint16 feeBps,address feeRecipient) = vaultFees.getFee(address(this));uint256 feeBasis = vaultFees.FEE_BASIS();// checks on inputs were done when building txemitRewardSent(address(this), referenceId, to, tokenAmounts, nativeTokenAmount, feeRecipient, feeBps);uint256 length = tokenAmounts.length;for (uint256 i =0; i < length; i++) {if (tokenAmounts[i].amount ==0) {continue; }uint256 tokenFee = (tokenAmounts[i].amount * feeBps) / feeBasis;if (tokenFee >0) {require(transferToken(tokenAmounts[i].token, feeRecipient, tokenFee),"VaultDelegate: token transfer to fee recipient failed" ); }require(transferToken(tokenAmounts[i].token, to, tokenAmounts[i].amount),"VaultDelegate: token transfer failed" ); }if (nativeTokenAmount ==0) {return; }uint256 nativeTokenFee = (nativeTokenAmount * feeBps) / feeBasis;if (nativeTokenFee >0) {// feeRecipient is trusted, we can skip this check// slither-disable-next-line arbitrary-send-eth,low-level-calls (bool successFee, ) = feeRecipient.call{ value: nativeTokenFee }("");require(successFee,"VaultDelegate: Failed to send ether to fee receiver"); }// slither-disable-next-line arbitrary-send-eth,low-level-calls (bool success, ) = to.call{ value: nativeTokenAmount, gas: gasToTarget }("");require(success,"VaultDelegate: Failed to send native token"); }
Impact Details
IMPACT:
Impacts in Scope:
Smart contract unable to operate due to lack of token funds
Contract fails to deliver promised returns, but doesn't lose value
Maybe, depends on your opinion too: Temporary freezing of funds >>> at least for the whitehat, in terms of not getting paid their reward when they expected to, a delay in reward payment due to the bug could potentially constitute as a temporary freezing of funds...?
if it wasn't for your sendRewardNoFees() function and your ability to withdraw both ERC20 & native tokens from the vaults, this bug would most certainly be either a critical severity or at least a high. Therefore I deem the severity at least medium.
For most/all sendReward() function calls the transaction will revert whenever the fee is non-zero, because once the fee has been sent the nativeTokenAmount value will be greater than the total funds remaining in the vault, and therefore the transaction will revert at this point:
The PoC tests: (make sure to check what specific cases I used in above test function). For example, in the default protocol test, it sends 1 ether as parameter value for native token value for queue reward tx and also for execute reward tx, but with my tests below I use 1.1 ether for the parameter value, which is the correct representation of the vault balance, not 1 ether.
Forge command used: forge test --contracts test/foundry/RewardTimelock.t.sol --mt testQueuesAndExecutesRewardTx -vvvvv
Test result: SUCCESS. Successfully sent reward to whitehat: ImmunefiGuard::checkAfterExecution(0xb02c9832b6b599b2da46eb47716a2afb30a542a718f426c2ca9ae05f21bbf709, true)
Test 3: default protocol test without any modifications and without any bugfix applied:
of course here the test passes because it cheat-code sends 1.1 ether into vault, and then it queues and executes reward transaction for only 1 ether instead of 1.1 ether, thereby allowing the bug to be overlooked because the fee is only 0.1 ether, so it works out perfectly to bypass the bug, which my tests above have proven to exist:
Test result: pass. No bugfix here but it passes due to the explanation above.
Lesson here: always double check your protocol unit/integration tests and make sure you cover all possible cases, including edge cases.
Also, your sendRewardNoFees() function has exactly the same code implementation except without the fee logic. This function works correctly because it doesnt have fees to account for.
BUGFIX SUGGESTION:
function sendReward( uint96 referenceId, address to, Rewards.ERC20Reward[] calldata tokenAmounts, uint256 nativeTokenAmount, uint256 gasToTarget ) external { require(gasToTarget <= UNTRUSTED_TARGET_GAS_CAP, "VaultDelegate: gasToTarget greater than max allowed"); (uint16 feeBps, address feeRecipient) = vaultFees.getFee(address(this)); uint256 feeBasis = vaultFees.FEE_BASIS(); // checks on inputs were done when building tx emit RewardSent(address(this), referenceId, to, tokenAmounts, nativeTokenAmount, feeRecipient, feeBps); uint256 length = tokenAmounts.length; for (uint256 i = 0; i < length; i++) { if (tokenAmounts[i].amount == 0) { continue; } uint256 tokenFee = (tokenAmounts[i].amount * feeBps) / feeBasis; if (tokenFee > 0) { require( transferToken(tokenAmounts[i].token, feeRecipient, tokenFee), "VaultDelegate: token transfer to fee recipient failed" ); } require(- transferToken(tokenAmounts[i].token, to, tokenAmounts[i].amount),+ transferToken(tokenAmounts[i].token, to, (tokenAmounts[i].amount - tokenFee)), "VaultDelegate: token transfer failed" ); } if (nativeTokenAmount == 0) { return; } uint256 nativeTokenFee = (nativeTokenAmount * feeBps) / feeBasis; if (nativeTokenFee > 0) { // feeRecipient is trusted, we can skip this check // slither-disable-next-line arbitrary-send-eth,low-level-calls (bool successFee, ) = feeRecipient.call{ value: nativeTokenFee }(""); require(successFee, "VaultDelegate: Failed to send ether to fee receiver"); } // slither-disable-next-line arbitrary-send-eth,low-level-calls- (bool success, ) = to.call{ value: nativeTokenAmount, gas: gasToTarget }("");+ (bool success, ) = to.call{ value: (nativeTokenAmount - nativeTokenFee), gas: gasToTarget }(""); require(success, "VaultDelegate: Failed to send native token"); }
