Biconomy - Smart Contract Wallet contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

On calling checkSignatures with v signature component as 0 (meaning it is a contract signature) then it is not validated that provided signature is actually signed by owner

Proof of Concept

1. Observe the checkSignatures function

function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view virtual {
        uint8 v;
        bytes32 r;
        bytes32 s;
        uint256 i = 0;
        address _signer;
        (v, r, s) = signatureSplit(signatures, i);
        //review
        if(v == 0) {
            // If v is 0 then it is a contract signature
            // When handling contract signatures the address of the contract is encoded into r
            _signer = address(uint160(uint256(r)));
...
} else if(v > 30) {
            // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
            // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
            _signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
require(_signer == owner, "INVALID_SIGNATURE");
        } else {
            _signer = ecrecover(dataHash, v, r, s);
require(_signer == owner, "INVALID_SIGNATURE");
        }

}

2. Observe that when v=0 then there is no check to validate if provided contract signature is signed by owner. This means

// Below check is missing when v=0

require(_signer == owner, "INVALID_SIGNATURE");

3. This means a carefully crafted sign by a fake contract would also pass the checkSignatures function and unauthorized transaction could get executed

Recommended Mitigation Steps

Revise the checkSignatures function as below:

function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view virtual {
        uint8 v;
        bytes32 r;
        bytes32 s;
        uint256 i = 0;
        address _signer;
        (v, r, s) = signatureSplit(signatures, i);
        //review
        if(v == 0) {
            // If v is 0 then it is a contract signature
            // When handling contract signatures the address of the contract is encoded into r
            _signer = address(uint160(uint256(r)));
...
} else if(v > 30) {
            // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
            // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
            _signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
        } else {
            _signer = ecrecover(dataHash, v, r, s);
        }
require(_signer == owner, "INVALID_SIGNATURE");
}

Lines of code

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

Vulnerability details

Impact

It is possible to replay the transaction by keeping the txHashData same as shown in POC

Proof of Concept

1. Victim calls execTransaction function with batchID 1

2. txHashData is calculated as below:

bytes memory txHashData =
                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
                    nonces[batchId]
                );

3. Since nonces[1] is used first time so value will be 0

4. Attacker replay this transaction with same _tx and refundInfo. He sets batchID as 2

5. txHashData is calculated and becomes same as Step 2. Reason is Attacker used same _tx and refundInfo and nonces2.

Recommended Mitigation Steps

Add one more field nonce (which increment for each executed transaction). This will ensure txHashData is always unique

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L54

Vulnerability details

Impact

Attacker can call handleOps function with lower gas. Any transaction, part of this bundle and requiring higher gas would fail. Even though the fault lies with Attacker, still the victim has to bear the fees

Proof of Concept

1. Attacker calls handleOps function with UserOperation U with gas say X

function handleOps(UserOperation[] calldata ops, address payable beneficiary) public {

        uint256 opslen = ops.length;
        UserOpInfo[] memory opInfos = new UserOpInfo[](opslen);

    unchecked {
        for (uint256 i = 0; i < opslen; i++) {
            _validatePrepayment(i, ops[i], opInfos[i], address(0));
        }

        uint256 collected = 0;

        for (uint256 i = 0; i < opslen; i++) {
            collected += _executeUserOp(i, ops[i], opInfos[i]);
        }

        _compensate(beneficiary, collected);
    } //unchecked
    }

2. The UserOperation contained one transaction T requiring higher gas say X+100
3. Transaction T fails since gas provided is not enough to cover this transaction and catch block is executed

function _executeUserOp(uint256 opIndex, UserOperation calldata userOp, UserOpInfo memory opInfo) private returns (uint256 collected) {
        uint256 preGas = gasleft();
        bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);

        try this.innerHandleOp(userOp.callData, opInfo, context) returns (uint256 _actualGasCost) {
           ...
        } catch {
            uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
            collected = _handlePostOp(opIndex, IPaymaster.PostOpMode.postOpReverted, opInfo, context, actualGas);
        }
    }

4. The catch block calculates the fees used and deduct the same from this victim account
5. Hence even without any victim's fault, his account is deducted with fee

Recommended Mitigation Steps

If Attacker has deliberately provided lower fees then the transaction should revert

Reference: https://github.com/eth-infinitism/account-abstraction/pull/162

Funds can stuck

Contract: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L35

Issue: Funds can stuck if withdraw address is contract itself

POC:

1. User calls withdrawTo function with withdrawAddress as StakeManager.sol contract itself

function withdrawTo(address payable withdrawAddress, uint256 withdrawAmount) external {
        DepositInfo storage info = deposits[msg.sender];
        require(withdrawAmount <= info.deposit, "Withdraw amount too large");
        info.deposit = uint112(info.deposit - withdrawAmount);
        emit Withdrawn(msg.sender, withdrawAddress, withdrawAmount);
        (bool success,) = withdrawAddress.call{value : withdrawAmount}("");
        require(success, "failed to withdraw");
    }

2. This transfer all User deposited ETH to the contract which process it via receive function

receive() external payable {
        depositTo(msg.sender);
    }

3. Since in this case msg.sender is contract itself so deposit is made for StakeManager.sol contract

4. Now there is no way to withdraw these funds

Recommendation: Kindly revise the receive function to disallow deposit by contract itself

receive() external payable {
require(msg.sender!=address(this), "Incorrect sender");
        depositTo(msg.sender);
    }

Stake condition can be revised to allow type(uint112).max

Contract: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L65

Issue: It seems that currently Staking amount is capped to type(uint112).max even though deposit cap is set to type(uint112).max. This means the Staking cap could be revised to also allow type(uint112).max

Recommendation: Kindly revise the addStake function

function addStake(uint32 _unstakeDelaySec) public payable {
...
        require(stake <= type(uint112).max, "stake overflow");
...
}

No way to determine execution status

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

Issue: If transaction execution via execTransaction does not success but has refundInfo.gasPrice != 0 then there is no way to know if the transaction never suceeded

POC:

1. execTransaction function is called for transaction T
2. Transaction T is executed but results in false as success

success = execute(_tx.to, _tx.value, _tx.data, _tx.operation, refundInfo.gasPrice == 0 ? (gasleft() - 2500) : _tx.targetTxGas);

3. Lets say refundInfo.gasPrice != 0 then below condition returns true and function is executed properly

require(success || _tx.targetTxGas != 0 || refundInfo.gasPrice != 0, "BSA013");
            // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
            uint256 payment = 0;
            // uint256 extraGas;
            if (refundInfo.gasPrice > 0) {
                //console.log("sent %s", startGas - gasleft());
                // extraGas = gasleft();
                payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
                emit WalletHandlePayment(txHash, payment);
            }
            // extraGas = extraGas - gasleft();
            //console.log("extra gas %s ", extraGas);
        }
    }

4. As we can see that in this case, there is no event mentioning that execution has failed which is incorrect

Recommendation: If success is false then execute an failure event mentioning the same