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

Potential Reentrancy Vulnerability in WithdrawalPool.withdraw Function

zibrasismail

Summary

The withdraw function in the WithdrawalPool contract performs an external call (token transfer) before updating the contract's state, potentially allowing for a reentrancy attack. While the use of SafeERC20 mitigates some risks, it doesn't completely eliminate the possibility of reentrancy with non-standard or malicious token implementations.

Vulnerability Details

The withdraw function in the WithdrawalPool contract follows this general structure:

Perform some checks

Calculate the amount to withdraw

Transfer tokens to the user using token.safeTransfer

Update the contract's state (e.g., reduce the user's balance)

This order of operations violates the checks-effects-interactions pattern, which is a best practice to prevent reentrancy attacks. The external call (token transfer) is made before the contract's state is updated, potentially allowing a malicious token contract to reenter the withdraw function and manipulate the contract's state.

While the use of SafeERC20's safeTransfer function provides some protection against reentrancy for standard ERC20 tokens, it doesn't guarantee protection against all possible token implementations, especially non-standard or malicious ones.

Impact

this vulnerability could allow an attacker to Withdraw more funds than they're entitled to, Manipulate the contract's state in unexpected ways and Potentially drain the contract of funds.

PoC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "./WithdrawalPool.sol";
​
contract MaliciousToken is ERC20 {
WithdrawalPool public withdrawalPool;
uint256 public withdrawCount;
​
constructor() ERC20("Malicious", "MAL") {
_mint(address(this), 1000000 * 10**18);
}
​
function setWithdrawalPool(address _withdrawalPool) external {
withdrawalPool = WithdrawalPool(_withdrawalPool);
}
​
function transfer(address recipient, uint256 amount) public override returns (bool) {
if (recipient == address(withdrawalPool) && withdrawCount < 2) {
withdrawCount++;
withdrawalPool.withdraw(new uint256[](1), new uint256[]());
}
return super.transfer(recipient, amount);
}
}

Tools Used

manual code review

Recommendations

Implement the checks-effects-interactions pattern in the withdraw function:

  • Perform all necessary checks

  • Update the contract's state (e.g., reduce the user's balance)

  • Perform the token transfer last

function withdraw(uint256[] calldata ids, uint256[] calldata amounts) external {
// Perform checks
require(ids.length == amounts.length, "Invalid input");
uint256 totalAmount = 0;
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
uint256 amount = amounts[i];
// Perform checks on id and amount
require(userBalances[msg.sender][id] >= amount, "Insufficient balance");
// Update state
userBalances[msg.sender][id] -= amount;
totalAmount += amount;
}
// Perform external call last
token.safeTransfer(msg.sender, totalAmount);
emit Withdrawn(msg.sender, totalAmount);
}

Consider implementing a reentrancy guard using OpenZeppelin's ReentrancyGuard contract as an additional layer of protection.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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