Summary
StakingPool::_updateStrategyRewards is unable to distribute rewards for all receivers if one strategy fee receiver reverts.
Vulnerability Details
This is because StakingPool::_updateStrategyRewards utilizes StakingPool::transferAndCallFrom to distribute fee amounts:
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
if (totalFeeAmounts > 0) {
uint256 sharesToMint = (totalFeeAmounts * totalShares) /
(totalStaked - totalFeeAmounts);
_mintShares(address(this), sharesToMint);
uint256 feesPaidCount;
for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
if (feesPaidCount == totalFeeCount - 1) {
=>[1] transferAndCallFrom(
address(this),
receivers[i][j],
balanceOf(address(this)),
"0x"
);
} else {
=>[2] transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x");
feesPaidCount++;
}
}
}
}
StakingPool::transferAndCallFrom:
function transferAndCallFrom(
address _sender,
=> address _to,
uint256 _value,
bytes memory _data
) internal returns (bool) {
_transfer(_sender, _to, _value);
if (isContract(_to)) {
contractFallback(_sender, _to, _value, _data);
}
return true;
}
StakingPool::transferAndCallFrom internally calls StakingPool::contractFallback (inherited from contracts/core/tokens/base/ERC677Upgradeable.sol) if the receiver is a contract
function contractFallback(
address _sender,
=> address _to,
uint256 _value,
bytes memory _data
) internal {
IERC677Receiver receiver = IERC677Receiver(_to);
=>[1] receiver.onTokenTransfer(_sender, _value, _data);
}
In [1] StakingPool will call receiver::onTokenTransfer, so if receiver onTokenTransfer reverts, or if receiver is a contract that doesnt implement onTokenTransfer and doesnt have a fallback function then it will revert too.
This will cause all strategy rewards distribution to fail for all receivers
The following PoC shows the described issue with the following scenario:
StakingPool set as a fee receiver a contract with a fallback that reverts
So when updateStrategyRewards is called all fee distribution process fails
Create contract file DoSCt.sol in contracts/core folder with following content
pragma solidity 0.8.15;
contract DoSCt {
constructor() {
}
fallback() external payable{
revert();
}
}
it('[R] SPool unable to distribute rewards if one receiver reverts', async () => {
const { signers, accounts, adrs, token, stakingPool, strategy, sdlPool, pp, withdrawalPool } = await loadFixture(
deployFixture
)
await strategy.setMaxDeposits(toEther(100));
await stakingPool.setUnusedDepositLimit(toEther(20));
await strategy.setFeeBasisPoints(1000)
console.log("[i] Deploying DoSCt: a contract with fallback that reverts");
console.log("[i] Setting accounts[0] and DoSCt as fee receivers, 5% each");
const dosct = (await deploy('DoSCt'));
const dosct_addr = await dosct.getAddress();
await stakingPool.addFee(accounts[0], 500)
await stakingPool.addFee(dosct.getAddress(), 500)
console.log("[i] Staking token with accounts 1,2,3 , amount 10 **18 each ");
await pp.connect(signers[1]).deposit(
toEther(10),
true,
['0x', '0x', '0x']
)
await pp.connect(signers[2]).deposit(
toEther(10),
true,
['0x', '0x', '0x']
)
await pp.connect(signers[3]).deposit(
toEther(10),
true,
['0x', '0x', '0x']
)
console.log("[i] Depositing 10 eth to strategy to simulate rewards");
await token.transfer(strategy.getAddress(), toEther(50))
console.log("\n=== Balances before updateStrategyRewards ===");
console.log(
"token.balanceOf(accounts[0]) ",
await token.balanceOf(accounts[0]),
"\nstToken.balanceOf(accounts[0]) ",
await stakingPool.balanceOf(accounts[0])
);
console.log(
"\ntoken.balanceOf(dosct_addr) ",
await token.balanceOf(dosct_addr),
"\nstToken.balanceOf(dosct_addr) ",
await stakingPool.balanceOf(dosct_addr)
);
console.log("\n[i] Calling updateStrategyRewards: will revert cause DoSCt will revert on its fallback function");
await expect(stakingPool.updateStrategyRewards([0], '0x')).to.be.reverted
console.log("\n === Balances after updateStrategyRewards ===");
console.log(
"token.balanceOf(accounts[0]) ",
await token.balanceOf(accounts[0]),
"\nstToken.balanceOf(accounts[0]) ",
await stakingPool.balanceOf(accounts[0])
);
console.log(
"\ntoken.balanceOf(dosct_addr) ",
await token.balanceOf(dosct_addr),
"\nstToken.balanceOf(dosct_addr) ",
await stakingPool.balanceOf(dosct_addr)
);
})
Impact
High, if one fee receiver reverts then no fee receiver can obtain his rewards
Tools Used
Manual Review
Recommendations
try catch cant be used because transferAndCallFrom is an internal function and try catch only work on external functions
However data used as a param in StakingPool::_updateStrategyRewards transferAndCallFrom is always '0x' so, its calling the receiver with an empty string as data, ie, there is no need to use transferAndCallFrom, just use transfer function
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
if (totalFeeAmounts > 0) {
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");
}
}
}
}
}
