0bing076 - H-1 Reentrancy Vulnerability in ZivoeTranches.depositBoth(uint256,address,uint256,address) (src/ZivoeTranches.sol#322-325)

[sherlock-admin3]

0bing076

medium

H-1 Reentrancy Vulnerability in ZivoeTranches.depositBoth(uint256,address,uint256,address) (src/ZivoeTranches.sol#322-325)

Summary

The ZivoeTranches contract is susceptible to a reentrancy attack due to the lack of proper reentrancy guard mechanisms in the depositBoth function. Additionally, the function does not follow the Checks-Effects-Interactions (CEI) pattern, which is crucial for preventing reentrancy attacks and ensuring the integrity of the contract's state. https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeTranches.sol?plain=1#L322#L323#L324 Label:(src/ZivoeTranches.sol) LoC #322-325 Severity:high

Vulnerability Detail

The depositBoth function in the ZivoeTranches contract is vulnerable to reentrancy attacks because it makes external calls to depositSenior and depositJunior without implementing proper CEI checks. This vulnerability allows an attacker to repeatedly call the function before the state is updated, potentially draining funds from the contract. Furthermore, the function does not follow the CEI pattern, which involves performing all checks (validations) before making any state changes or external calls. This lack of pattern can lead to unexpected behaviour and potential loss of funds.

Impact

If exploited, this vulnerability could allow an attacker to drain funds from the ZivoeTranches contract by repeatedly calling the depositBoth function before the contract's state is updated. This could have significant financial implications for the contract and its users, potentially leading to loss of trust and operational disruption.

Code Snippet

function depositBoth(uint256 amountSenior, address assetSenior, uint256 amountJunior, address assetJunior) external {
        depositSenior(amountSenior, assetSenior);
        depositJunior(amountJunior, assetJunior);
    }

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, the ZivoeTranches contract should follow the CEI pattern by performing all checks and state updates before making external calls. This ensures that the contract's state is consistent and prevents reentrancy attacks.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract ZivoeTranches is ReentrancyGuard {
    // rest of the code here

    function depositBoth(uint256 amountSenior, address assetSenior, uint256 amountJunior, address assetJunior) external nonReentrant {
        // Input Validation
        require(amountSenior > 0 && amountJunior > 0, "Deposit amounts must be greater than 0");
        require(assetSenior != address(0) && assetJunior != address(0), "Asset addresses must be valid");

        // Effects
        balances[msg.sender] += amountSenior + amountJunior;

        // Interactions
        depositSenior(amountSenior, assetSenior);
        depositJunior(amountJunior, assetJunior);
    }
}

Use the SafeERC20 library for safe token transfers to prevent unexpected behaviour and ensure the integrity of the contract's state. By addressing these issues, the ZivoeTranches contract can significantly enhance its security, protecting it from reentrancy attacks and ensuring the integrity of its state.

Proof of Concept We'll simulate an attack scenario where an attacker contract exploits the vulnerability. This POC will demonstrate how an attacker could drain funds from the ZivoeTranches contract by repeatedly calling the depositBoth function before the contract's state is updated. Attacker Contract

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract Attacker {
    using SafeERC20 for IERC20;

    ZivoeTranches public zivoeTranches;
    IERC20 public token;

    constructor(address _zivoeTranches, address _token) {
        zivoeTranches = ZivoeTranches(_zivoeTranches);
        token = IERC20(_token);
    }

    // Fallback function to receive Ether
    receive() external payable {}

    function attack(uint256 amount) external {
        // Approve the ZivoeTranches contract to spend the attacker's tokens
        token.safeApprove(address(zivoeTranches), amount);

        // Call the vulnerable depositBoth function
        zivoeTranches.depositBoth(amount, address(token), amount, address(token));
    }

    // This function is called by the ZivoeTranches contract during the deposit process
    function depositSenior(uint256 amount, address asset) external {
        // Attempt to withdraw the tokens again
        token.safeTransfer(msg.sender, amount);
    }
}

The attacker contract is initialized with the address of the ZivoeTranches contract and the address of the token to be drained. The attack function is called by the attacker. It approves the ZivoeTranches contract to spend a specified amount of tokens on behalf of the attacker. Then, it calls the depositBoth function of the ZivoeTranches contract, attempting to exploit the reentrancy vulnerability. Reentrancy Exploit: When the depositBoth function calls depositSenior, the Attacker contract's depositSenior function is triggered. Since the depositBoth function has not yet updated its state (effects), the attacker can call depositSenior again before the first call is finished, draining more tokens than intended. The attacker contract includes a fallback function to receive Ether, which could be used to fund the attack. To help mitigate this vulnerability, the ZivoeTranches contract could follow the CEI pattern by performing all checks and state updates before making external calls. We've provided the POC to demonstrate the potential impact of a reentrancy vulnerability and highlights the importance of implementing proper security measures in smart contracts.

[pseudonaut]

Both functions underneath have nonreentrant?

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, report statements are incorrect, these are just 2 functions one after another, nothing can be reentered here