Loss of funds if mistakenly sent to the implementation contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L550

Vulnerability details

The SmartAccount contract that is used as the implementation contract behind the Proxy defines the receive function.

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L550

receive() external payable {}

This will allow the implementation contract to receive ETH.

Impact

A user may mistakenly send funds to the implementation contract. These transactions will succeed, as the contract implements the receive() payable function.

Any ETH sent to the implementation contract will be lost, as this contract represents the implementation logic and not the proper user's wallet (the proxy).

PoC

The following test illustrates the issue.

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }

    function test_SmartAccount_SendEthToImplementation() public {
        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        uint256 amount = 1 ether;
        (bool sent,) = address(implementation).call{value: 1 ether}("");
        require(sent);

        assertEq(address(implementation).balance, amount);
        assertEq(address(wallet).balance, 0);
    }
}

Recommendation

Remove the receive function from the SmartAccount contract. The receive function should only be implemented in the Proxy contract.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Overinflated severity