Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Wrong calldata argument provided to `transferAndCallFrom` in `WithdrawPool::_updateStrategyRewards` results in a revert in `PriorityPool::onTokenTransfer`

falsegenius

Vulnerability Details

The way WithdrawPool::_updateStrategyRewards works, it distributes rewards/fees based on balance changes in strategies since the last update by,

  1. Summing up rewards and fees across strategies.

  2. Updating totalStaked if there was a net change in deposits.

  3. Calulating fees if net positive rewards were earned

  4. Distribute fees to receivers if there are any.

The 4th step where it makes use of transferAndCallFrom to transfer the ERC677 fee amount and the issue lies in how it makes use of it.The transferAndCallFrom calls onTokenTransfer on receiver if the receiver is a smart contract. PriorityPool has a onTokenTransfer function to accomodate this, and it starts by decoding the callldata,

function onTokenTransfer(address _sender, uint256 _value, bytes calldata _calldata) external {
if (_value == 0) revert InvalidValue();
​
@> (bool shouldQueue, bytes[] memory data) = abi.decode(_calldata, (bool, bytes[]));
...
...
}

So essentially, the calldata provided needs to be an encoded combination of a bool and bytes[], but the value provided is "0x" that would always result in a revert,

transferAndCallFrom(address(this), receivers[i][j], balanceOf(address(this)), "0x")

The PriorityPool::onTokenTransfer has an if-else statement that calls _withdraw if the onTokenTransfer is called by StakingPool. This means PriorityPool expects transfers ERC677 transfers from StakingPool would cause revert here.

Proof of Concept

Place the following test in staking-pool.test.ts

​
it("onTokenTransfer reverts on 0x calldata", async () => {
const { accounts, adrs, stakingPool, strategy1, strategy3, token, strategy2, stake } =
await loadFixture(deployFixture)
​
let priorityPool = (await deployUpgradeable('PriorityPool', [
token.target,
stakingPool.target,
accounts[0],
0,
0,
])) as PriorityPool
​
await stakingPool.addFee(accounts[0], 500)
// Adding PriorityPool as a receiver
await stakingPool.addFee(priorityPool.target, 500)
​
await strategy1.setFeeBasisPoints(1000)
await strategy3.setFeeBasisPoints(1000)
​
await stake(1, 2000)
await stake(2, 1000)
await stake(3, 2000)
await token.transfer(adrs.strategy1, toEther(1000))
await token.transfer(adrs.strategy3, toEther(600))
await strategy2.simulateSlash(toEther(300))
​
// reverts
await expect(stakingPool.updateStrategyRewards([0, 1, 2], '0x')).to.reverted;
})
​

Impact

The WithdrawPool::_updateStrategyRewards would always revert with receiver being PriorityPool and sender being StakingPool. This disrupts the workflow of the protocol by preventing proper fee distribution.

Tools Used

Manual Review & Hardhat Testing

Recommendations

Consider making the following changes,

​
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
int256 totalRewards;
uint256 totalFeeAmounts;
uint256 totalFeeCount;
address[][] memory receivers = new address[][]();
uint256[][] memory feeAmounts = new uint256[][]();
​
...
...
...
​
// distribute fees to receivers if there are any
if (totalFeeAmounts > 0) {
uint256 sharesToMint = (totalFeeAmounts * totalShares) /
(totalStaked - totalFeeAmounts);
_mintShares(address(this), sharesToMint);
​
uint256 feesPaidCount;
+ bool shouldQueue = true;
+ bytes[] memory data = new bytes[]();
+ data[0] = bytes("");
+ bytes memory _calldata = abi.encode(shouldQueue, data);
for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
if (feesPaidCount == totalFeeCount - 1) {
+ transferAndCallFrom(address(this), receivers[i][j], balanceOf(address(this)), _calldata);
- transferAndCallFrom(address(this), receivers[i][j], balanceOf(address(this)),"0x");
} else {
+ transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], _calldata);
- transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x");
feesPaidCount++;
}
}
}
}
​
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

[INVALID] Wrong calldata argument provided to `transferAndCallFrom` in `WithdrawPool::_updateStrategyRewards` results in a revert in `PriorityPool::onTokenTransfer`

Appeal created

inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[INVALID] Wrong calldata argument provided to `transferAndCallFrom` in `WithdrawPool::_updateStrategyRewards` results in a revert in `PriorityPool::onTokenTransfer`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.