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

Vulnerabilities in Fee Distribution to Smart Contract Recipients in a Loop

0xsurena

Summary

The StakingPool contract distributes fees to recipients in a loop using the transferAndCallFrom function. This process works efficiently when recipients are standard addresses, but introduces significant vulnerabilities when a recipient is a smart contract. Specifically:

  1. If a smart contract recipient does not implement the onTokenTransfer function, the transaction will fail.

  2. Even if the onTokenTransfer function is implemented, it can contain malicious or faulty logic, causing the transaction to revert.

  3. If the recipient is an upgradeable contract, future modifications can introduce errors or malicious behavior, leading to failure in fee distribution.

Vulnerability Details

The transferAndCallFrom function, used to send fees in the loop, calls contractFallback if the recipient is a smart contract:

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L592

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;
}

If the recipient is a contract, contractFallback is called, which invokes the onTokenTransfer function in the recipient contract:

function contractFallback(
address _sender,
address _to,
uint256 _value,
bytes memory _data
) internal {
IERC677Receiver receiver = IERC677Receiver(_to);
receiver.onTokenTransfer(_sender, _value, _data);
}

Loop in _updateStrategyRewards:

The fee distribution is performed in a loop. If any recipient's contract fails, the entire process reverts, causing no recipient to receive their fee.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L582

for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x");
}
}

Scenario 1: Missing onTokenTransfer

If a smart contract does not implement the onTokenTransfer function, the contractFallback call will fail, reverting the entire transaction. This leads to the failure of fee distribution to all recipients.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/tokens/base/ERC677Upgradeable.sol#L52

Scenario 2: Malicious or Faulty onTokenTransfer

Even if the onTokenTransfer function is implemented, there is no guarantee that it is safe. Malicious or faulty code within this function could intentionally revert the transaction or introduce gas limits, causing the entire fee distribution process to fail.

function onTokenTransfer(
address _sender,
uint256 _value,
bytes memory _data
) external {
// Malicious or faulty code could cause a revert here, impacting the entire distribution
}

Scenario 3: Upgradeable Contracts

Upgradeable contracts introduce additional risk. Even if the onTokenTransfer function is properly implemented at the time of integration, the contract owner can later upgrade the contract and either remove the onTokenTransfer function or introduce malicious logic. This can cause the entire fee distribution to fail when interacting with the updated contract.

Impact

If any smart contract recipient causes a failure (either by missing the onTokenTransfer function or having a faulty implementation), the entire fee distribution process for all recipients will revert. This means that even valid recipients will not receive their fees.

Tools Used

Manually

Recommendations

Implementing a mechanism like try-catch can help manage unexpected failures. this will ensure that even if one contract causes an error, the rest of the recipients will still receive their fees.

for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
try transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x") {
// Fee transfer succeeded
} catch {
// Log the failure and continue with other recipients
}
}
}
try receiver.onTokenTransfer(_sender, _value, _data) {
// Success logic
} catch {
// Handle failure, log an event or skip this recipient
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

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

`updateStrategyRewards` is not called before adding & updating the fees

It should be called with try and catch to avoid DOS by receiver.

Support

FAQs

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