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

Incompatibility between StakingPool::_updateStrategyRewards and PriorityPool::onTokenTransfer implementation made StakingPool unable to send staked tokens (stLink) to PriorityPool

0xw3

Summary

Incompatibility between StakingPool::updateStrategyRewards and PriorityPool::onTokenTransfer implementation made StakingPool unable to send staked tokens (stLink) to PriorityPool and transaction will always revert

Vulnerability Details

PriorityPool::onTokenTransfer implementation expects that contract who calls it sends a boolean and a byte array as calldata when sending tokens.
onTokenTransfer function will try to decode calldata to a boolean and byte array:

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[])); // trying to decode calldata as a boolean and bytearray

However, when StakingPool sends stLink on updateStrategyRewards it sends 0x as calldata as seen in [1] and [2]:

function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
//... snippet ... sum up rewards and fees across strategies ...
//... snippet ... fee and rewards calculations ...
​
// distribute fees to receivers if there are any fees
if (totalFeeAmounts > 0) {
//... snippet... mint calculation , and stLink minting for rewards
_mintShares(address(this), sharesToMint);
//... snippet Fee distribution
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)),
=>[1] "0x" // Bytes send to onTokenTransfer
);
} else {
transferAndCallFrom(
address(this), receivers[i][j], feeAmounts[i][j],
=>[2] "0x" // Bytes send to onTokenTransfer
);
feesPaidCount++;
}
}}}
//...snippet
}

From StakingPool::transferAndCallFrom definition (inherited from contracts/core/tokens/base/ERC677Upgradeable.sol) , we see StakingPool::updateRewards call set param data always as '0x' [1] [2]:

function transferAndCallFrom(
address _sender, address _to, uint256 _value,
bytes memory _data // <= data = '0x'
) internal returns (bool) {
[A] _transfer(_sender, _to, _value);
if (isContract(_to)) {
[B] contractFallback(_sender, _to, _value, _data);
}
return true;
}

In [A] transferAndCallFrom sends an amount of value token to _to
And if _to is a contract, then in [B] it will call contractFallback function, with data='0x':
Finally contractFallback function definition (contracts/core/tokens/base/ERC677Upgradeable.sol) calls receiver onTokenTransfer function with data (in this case '0x') as a parameter :

function contractFallback(
address _sender, address _to, uint256 _value,
bytes memory _data // <= '0x'
) internal {
IERC677Receiver receiver = IERC677Receiver(_to);
=> receiver.onTokenTransfer(_sender, _value, _data);
}

So, this triggers PriorityPool::onTokenTransfer and will fail always cause will try to decode '0x' (bytes 0x3078) as a boolean and a bytearray

This made StakingPool unable to send tokens to PriorityPool.
And PriorityPool unable to receive them from StakingPool.
Also note that StakingPool is one of the only two addresses authorized to call PriorityPool::onTokenTransfer, the other one is token address :

function onTokenTransfer(address _sender, uint256 _value, bytes calldata _calldata) external {
//... snippet
=> if (msg.sender == address(token)) {
_deposit(_sender, _value, shouldQueue, data);
=> } else if (msg.sender == address(stakingPool)) {
// ... snippet
} else {
=> revert UnauthorizedToken();
}
}

The following PoC show the issue described above, with the following scenario:

  1. StakingPool sets PriorityPool as a fee receiver

  2. An user deposits to StakingPool

  3. Link is transfered to strategy to simulate a net positive balance change

  4. StakingPool::updateRewards is called triggering a call to PriorityPool::onTokenTransfer

  5. Tx fails and will always fail and fee cannot be distributed to fee receivers
    Create the following test case in test/core/priorityPool/priority-pool.test.ts

it('[R] PriorityPool onTokenTransfer wrong implementation', async () => {
const { signers, accounts, adrs, token, stakingPool, strategy, sdlPool, pp, withdrawalPool } = await loadFixture(
deployFixture
)
let etherAmount = toEther(20);
let index = 3;
let signer = signers[index]
let account = accounts[index]
let priorityPool = await pp.getAddress();
​
console.log("[i] Adding priority pool as a StakingPool fee receiver")
await stakingPool.addFee(priorityPool,500);
​
console.log(`[i] Staking ${etherAmount}`);
await pp.connect(signer).deposit(
etherAmount, true, ['0x', '0x', '0x']
)
​
console.log(`[i] Transfering ${etherAmount} to strategy to simulate rewards`)
await token.transfer(adrs.strategy, toEther(10))
​
console.log("[i] Calling stakingPool.updateStrategyRewards")
console.log("\tStakingPool will try to send stLink to PriorityPool and call PPool::onTokenTransfer");
console.log("[i] This tx reverts: (fragment of error): ");
​
// this tx reverts, but cant be catched with expect().to.be.reverted
try {
await stakingPool.updateStrategyRewards([0], '0x')
}catch(error){
console.log(error.toString().substring(0,800));
}
})

Observe the transaction fails because of Incompatibility between StakingPool::updateStrategyRewards and PriorityPool::onTokenTransfer

Impact

StakingPool is unable to send tokens to PriorityPool via updateStrategyRewards
DoS on StakingPool::updateStrategyRewards

Tools Used

Manual Review

Recommendations

Because StakingPool::transferAndCallFrom always use a hardcoded empty string as a data paramater its better to remove usage of transferAndCallFrom and use transfer instead:

function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
// ... snippet
if (totalFeeAmounts > 0) {
// snippet
for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
if (feesPaidCount == totalFeeCount - 1) {
(bool success) = transfer(receivers[i][j],balanceOf(address(this))
require(sucess, "couldnt transfer to receiver");
} else {
(bool success) = transfer(receivers[i][j],feeAmounts[i][j]);
require(sucess, "couldnt transfer to receiver");
}
}
}
}
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 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

galturok Judge
10 months ago
0xw3 Submitter
10 months ago
inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 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.