code-423n4 / 2023-10-brahma-findings

8 stars 7 forks source link

registerWallet() does not validate the sender #415

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/WalletRegistry.sol#L35

Vulnerability details

Issue

registerWallet() in WalletRegistry.sol does not guarantee that the sender is the safe deployer. registerWallet() should be called from the safe deployer, in the context of deployConsoleAccount() // Register Wallet /// @dev This function is being packed as a part of multisend transaction as, safe internally performs // a delegatecall during initializer to the target contract, so direct call doesnt work. Multisend is // supposed to be delegatecall txns[0] = Types.Executable({ callType: Types.CallType.CALL, target: AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH), value: 0, data: abi.encodePacked(WalletRegistry.registerWallet.selector) });

Impact

SafeDeployer will call enableModule/setGuard for an arbitrary sender, which hasn't been deployed as a Console Account. This may lead to confusion of privileged access roles. in SafeDeployer // Enable Brhma Console account as module on sub Account txns[0] = Types.Executable({ callType: Types.CallType.DELEGATECALL, target: safeEnabler, value: 0, data: abi.encodeCall(IGnosisSafe.enableModule, (_consoleAccount)) });

Fix

`/**
 * @notice Registers a wallet
 * @dev Can only be called by safe deployer or the wallet itself
 */
function registerWallet() external {
    if (msg.sender != AddressProviderService._getAuthorizedAddress(_SAFE_DEPLOYER_HASH)) revert InvalidSender();
    if (isWallet[msg.sender]) revert AlreadyRegistered();
    if (subAccountToWallet[msg.sender] != address(0)) revert IsSubAccount();
    isWallet[msg.sender] = true;
    emit RegisterWallet(msg.sender);
}`

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #17

c4-judge commented 1 year ago

alex-ppg marked the issue as unsatisfactory: Invalid