Biconomy - Smart Contract Wallet contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.) A malicious user might self-destruct the SmartAccount contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. A malicious user might self-destruct the SmartAccount contract in the following way:

1. First, the init() function can be called both from a proxy and by anybody. The later should be disabled during contract deployment using _disableInitializers(). Unfortunately, this is not disabled, as a result, anybody can call the init() function.
2. Suppose a malicious user call init() and set up a malicous _entryPointAddress and takes over the ownership.
3. The malicious entryPoint contract then calls execFromEntryPoint() with a dest and func that will perform a selfdestruct operation via the delegate call (operation == DelegateCall).
4. As a result, the SmartAccount contract will be destroyed due to the selfdestruct.

Tools Used

Remix

Recommended Mitigation Steps

The fix is to include a constructor that will disable the call of init():

constructor()
{
         _disableInitializers()
}

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding deployCounterFactualWallet(() does not check whether the caller is the owner

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33

deployCounterFactualWallet(() does not check whether the caller is the owner, as a result, any user might this function and create a SmartAccunt for another user.

Tools Used

Remix

Recommended Mitigation Steps

We can add a check in the beginning of the function.

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){

        if(owner != msg.sender) revert NotByOwner();

        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }
        require(address(proxy) != address(0), "Create2 call failed");
        // EOA + Version tracking
        emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

QA1. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L397 There is a typo here, the comparison should be against type(uint128).max instead of type(uint120).max since the documentation says "validate all numeric values in userOp are well below 128 bit".

require(maxGasValues <= type(uint128).max, "AA94 gas values overflow");

QA2: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/BasePaymaster.sol#L75 The modifier onlyOwner is not necessary, just like the deposit() function.

QA3: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/BasePaymaster.sol#L99 Zero address check is necessary for withdrawAddress to avoid losing funding to the zero address.

QA4: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/common/Singleton.sol#L2 Lock all contracts to the most recent version of Solidity, 0.8.17.

QA5. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/handler/DefaultCallbackHandler.sol#L55-L61 The function should also indicate that it supports the interface of ERC777TokensRecipient as well.

function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
        return
            interfaceId == type(ERC1155TokenReceiver).interfaceId ||
            interfaceId == type(ERC721TokenReceiver).interfaceId ||
            interfaceId == type(ERC777TokensRecipient).interfaceId || 
            interfaceId == type(IERC165).interfaceId;
    }

QA6. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L115 Zero address check for withdrawAddress is necessary to avoid losing funding.

QA7. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/PaymasterHelpers.sol#L36 paymasterAndData[20:] should be paymasterAndData instead:

(address paymasterId, bytes memory signature) = abi.decode(paymasterAndData, (address, bytes));

QA8. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/libs/MultiSendCallOnly.sol#L21 There is a for loop which contains a low-level call, we need to avoid the msg.value reuse vulnerability problem, see the following for more details: https://blog.trailofbits.com/2021/12/16/detecting-miso-and-opyns-msg-value-reuse-vulnerability-with-slither/ The fix is that we need to make sure this function has no modifier payable.

 function multiSend(bytes memory transactions) public{
}

G1. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L58 Adding unchecked to save gas since overflow is not possible due to previous check.

unchecked{
     paymasterIdBalances[msg.sender] -= amount;
}

G2. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L118 Adding unchecked to save gas since overflow is not possible due to previous check.

unchecked{
 info.deposit = uint112(info.deposit - withdrawAmount);
 
}

G3. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L58 Adding unchecked to save gas since overflow is not possible due to previous check.

unchecked{
     paymasterIdBalances[msg.sender] -= amount;
}

G4. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L81 Using userOp.sender can save gas.

function getHash(UserOperation calldata userOp)
    public pure returns (bytes32) {
        //can't use userOp.hash(), since it contains also the paymasterAndData itself.
        return keccak256(abi.encode(
                userOp.sender,
                userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.callGasLimit,
                userOp.verificationGasLimit,
                userOp.preVerificationGas,
                userOp.maxFeePerGas,
                userOp.maxPriorityFeePerGas
            ));
    }

G5. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L99 There is no need for this line since requiredPreFund is used in the body of the function.

G6. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L473 Adding unchecked to save gas since overflow is not possible due to previous check.

unchecked{
     uint256 refund = opInfo.prefund - actualGasCost;
}

G7. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L484-L494 The implementation can be simplified because min(maxFeePerGas, maxPriorityFeePerGas + block.basefee) is always the correct answer even when maxFeePerGas == maxPriorityFeePerGas.

function getUserOpGasPrice(MemoryUserOp memory mUserOp) internal view returns (uint256) {
    unchecked {
        return min(mUserOp.maxFeePerGas, mUserOp.maxPriorityFeePerGas + block.basefee);
    }
    

G8. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L336 Adding unchecked to save gas since overflow is not possible due to previous check.

senderInfo.deposit = uint112(deposit - requiredPrefund);

G9. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L362 Adding unchecked to save gas since overflow is not possible due to previous check.

 paymasterInfo.deposit = uint112(deposit - requiredPreFund);