code-423n4 / 2024-06-panoptic-findings

1 stars 0 forks source link

Use of delegatecall Inside Loop in Payable Function within Multicall contract #50

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/base/Multicall.sol#L12-L36

Vulnerability details

Impact

Detailed description of the impact of this finding.

Contract: Multicall.sol

Function: multicall(bytes[])

Lines:

12-36

Issue: Use of delegatecall Inside Loop in Payable Function

The implementation of delegatecall inside a loop within a payable function is a pattern that can lead to significant security risks, including DOS attacks and unexpected behaviour due to external control over contract logic. This pattern is particularly dangerous because it can be exploited to drain gas.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The function multicall(bytes[]) contains the following line of code:

(success, result) = address(this).delegatecall(data[i]);

This line indicates that multiple delegate calls are made within a loop. If any of the called contracts are malicious or compromised, they could potentially hijack the control flow, leading to severe consequences.

Test File Name: test/foundry/core/Multicall.t.sol
Prerequisite: Remove the "abstract" prefix from the Multicall.sol file contract name.
And save the below foundry test code in the file path test/foundry/core/Multicall.t.sol of your github codebase.
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;

import "ds-test/test.sol";
import {Multicall} from "@contracts/base/Multicall.sol"; 

contract DOSAttackTest is DSTest {
    Multicall multicall;

    function setUp() public {
        multicall = new Multicall();
    }

    function testDOSAttack() public {
        // Prepare a large array of calldata that would exhaust gas limits
        uint256[] memory datae = new uint256[](1);
        datae[0] = uint256(115792089237316195423570985008687907853269984665640564039457584007913129639935);
        bytes[] memory data = new bytes[](1); // Adjust the size as needed for the test
        data[0] = abi.encodePacked(datae);
        for (uint256 i = 0; i < data.length; i++) {
            // Fill the array with calldata that calls a function in the Multicall contract
            data[i] = abi.encodeWithSelector(Multicall.multicall.selector);
        }

        // Expect the transaction to fail due to out of gas
        (bool success, bytes memory returnedData) = address(multicall).call{gas: 1000000}(abi.encodeWithSelector(Multicall.multicall.selector, data));

        assertTrue(!success, "DOS attack should fail the transaction");
    }
}
forge test -vvvvv --match-contract DOSAttackTest  --fork-url "https://eth-mainnet.g.alchemy.com/v2/{TOKEN}"
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for test/foundry/core/Multicall.t.sol:DOSAttackTest
[PASS] testDOSAttack() (gas: 8475)
Traces:
  [198337] DOSAttackTest::setUp()
    ├─ [160808] → new Multicall@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 803 bytes of code
    └─ ← [Stop] 

  [8475] DOSAttackTest::testDOSAttack()
    ├─ [1272] Multicall::multicall([0xac9650d8])
    │   ├─ [140] Multicall::multicall() [delegatecall]
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.04s (373.83µs CPU time)

Ran 1 test suite in 2.16s (1.04s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

In this test, we’re trying to simulate a DOS attack by sending a large number of calls through the multicall function with a limited amount of gas. The expectation is that the transaction will fail because it runs out of gas, which would indicate a potential DOS vulnerability in the contract.

Tools Used

Manual review, Slither, and Foundry for POC.

Recommended Mitigation Steps

Avoid delegatecall in Loops: Refactor the function to remove the use of delegatecall within loops.

Assessed type

DoS

c4-judge commented 1 month ago

Picodes marked the issue as unsatisfactory: Invalid