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

Reentracy vulnerability inside Vault.sol

0bingo76

Summary

Possible vulnerability allows an attacker to exploit the function by recursively calling it before the initial transaction is completed, potentially draining the contract of funds beyond the intended amount.

Vulnerability Details

The function performs an external call to stakeController.unstake(_amount) before updating the contract's state or transferring tokens. This order of operations creates a window for reentrancy attacks, where an attacker can recursively call the withdraw function before the first call completes. Specifically, the stakeController.unstake(_amount) call creates the reentrancy opportunity in the following ways:

  • The unstake function is an external call to another contract. During this call, control is temporarily passed to the stakeController contract, which may have malicious code.

  • At the point of the unstake call, the Vault contract's state has not been updated to reflect the withdrawal. This means that if the stakeController contract calls back into the Vault, it will see the pre-withdrawal state.

  • If the stakeController contract is malicious or compromised, it could include logic in its unstake function to call back into the Vault's withdraw function before completing the unstaking process.

  • Due to the state not being updated, each recursive call to withdraw would process as if it were the first withdrawal, potentially allowing multiple withdrawals of the same funds.

  • This vulnerability is particularly dangerous because the Vault contract trusts the stakeController to behave correctly. If an attacker can control or manipulate the stakeController, they can exploit this trust to drain funds from the Vault.

    https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/Vault.sol#L76-L79

Impact

Potential loss of all funds stored in the `Vault` contract. Compromised balance tracking and intended behavior of the contract.
There is a potential cascading effects on other contracts and protocols relying on the `Vault`.
Bob and Alice Scenario
​
## Bob and Alice are two users interacting with the `Vault` contract.
​
Bob is a legitimate user who deposits tokens into the `Vault`,while Alice is a malicious actor who aims to exploit the reentrancy vulnerability.
​
### Step-by-Step Scenario
​
Bob deposits 10 tokens into the `Vault` contract, which he believes is secure. The `Vault` contract records this deposit.
​
Alice deploys the `MaliciousVaultController` contract, which is designed to exploit the reentrancy vulnerability in the `Vault`.
​
Alice sets herself as the `vaultController` by calling the `setVaultController` function in the `Vault`, replacing Bob's address.
​
Alice calls the `initiateAttack` function in her malicious contract, requesting a withdrawal of 1 token.
​
When the `withdraw` function is executed, it calls `stakeController.unstake`, and then attempts to transfer tokens back to Alice. During this transfer, Alice's fallback function is triggered, allowing her to call `withdraw` again.
​
Alice continues to call `withdraw` recursively, draining the `Vault` of its funds. In a matter of seconds, she could withdraw significantly more than her initial deposit, leaving Bob and other legitimate users with nothing.
​
Bob discovers that the `Vault` is empty, and his funds are lost due to Alice's exploit. The trust in the `Vault` contract is severely damaged, affecting all users.

Tools Used

Manual code review

Recommendations

To address the reentrancy vulnerability and improve overall security, consider these modifications:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract Vault is ReentrancyGuard {using SafeERC20 for IERC20;
modifier onlyVaultController() {
require(msg.sender == vaultController, "Not vault controller");
_;
}
​
function withdraw(uint256 _amount) external virtual onlyVaultController nonReentrant {
require(balances[msg.sender] >= _amount, "Insufficient balance");
balances[msg.sender] -= _amount;
stakeController.unstake(_amount);
token.safeTransfer(vaultController, _amount);
emit Withdrawn(msg.sender, _amount);
}
​

Reorder operations in the `withdraw` function to update the contract's state before making external calls.

Implement OpenZeppelin's `ReentrancyGuard` to prevent recursive calls to sensitive functions.

Updates

Lead Judging Commences

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

Support

FAQs

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