GHSA-97cv-x867-6xhm

Description

The VM built-in function KleverUpdateAccountPermission (registered always-active, creator.go:381-390 / core/vmconstants.go:234) rewrites an account's entire permission set. Its authorization check uses vmInput.RecipientAddr attacker-controlled instead of the authenticated vmInput.CallerAddr. The sibling handler kleverChangeOwnerAddress.go:86 uses vmInput.CallerAddr correctly, so the safe pattern exists in-repo; this handler deviates. The native transaction path (txProcess.go:833) is safe it uses tx.GetSender().

Mechanism:

1. Wrong variable: CallerAddr is never referenced in the handler; auth is contractHasValidPermission(target.GetPermissions(), RecipientAddr), which returns true if RecipientAddr is a signer with Weight >= Threshold in the target account's permissions and the permission grants UpdateAccountPermissionContractType.
2. RecipientAddr is attacker-controlled: when a contract calls a built-in via ExecuteOnDestContextWithTypedArgs (baseOps.go:1967), prepareIndirectContractCallInput (baseOps.go:2485) sets RecipientAddr = destination (contract-chosen) and CallerAddr = the calling contract. The blockchain hook (blockChainHook.go:454/467) dispatches on input.Function and passes the input through unchanged; no guard forces RecipientAddr == CallerAddr and there is no SC-destination validation on this path.
3. Self-signer default satisfies the check: createDefaultOwnerPermission (accounts.go:1848) makes an account its own signer (weight 1, threshold 1, Owner type), and CheckPermissionGrantedForContracts returns true for Owner, so contractHasValidPermission(V.perms, V) == true. (More generally, RecipientAddr can be set to any of V's signer addresses meeting threshold all public on-chain.) Accounts with no stored permissions have empty GetPermissions() and are immune.
4. Overwrite is unrestricted: UpdatePermission(V, attackerContract) (accounts.go:1863) replaces V's permission set with attacker-supplied signers; if the attacker supplies an Owner-type permission, no default is appended and V's prior control is fully evicted.

Code walkthrough

(a) The vulnerable handler — core/kapp/builtInFunctions/kleverUpdateAccountPermission.go:

func (e *kleverUpdateAccountPermission) ProcessBuiltinFunction(vmInput *vmcommon.ContractCallInput) (*vmcommon.VMOutput, error) {
    ...
    address := vmInput.NextArg()                       // Arguments[0] — attacker-chosen target account V
    contract, err := e.getUpdateAccountPermissionContract(vmInput) // Arguments[1] — attacker-chosen new permissions
    ...
    acc, err := e.accountsCacher.LoadUser(address)      // loads V
    ...
    // BUG: authorizes against vmInput.RecipientAddr (attacker-controlled), NOT vmInput.CallerAddr
    if !e.contractHasValidPermission(acc.GetPermissions(), vmInput.RecipientAddr) {   // L91
        return nil, errors.New("invalid permission operation")
    }
    // overwrites V's entire permission set with attacker-supplied signers
    resultCode, err := e.kappController.GetAccountsKApp().UpdatePermission(address, contract)
    ...
}

(b) The check just name-matches recipientAddr against V's own signers — same file:

func (e *kleverUpdateAccountPermission) contractHasValidPermission(permissions []*state.Permission, recipientAddr []byte) bool {
    for _, permission := range permissions {
        for _, signer := range permission.Signers {
            if !bytes.Equal(signer.Address, recipientAddr) {   // recipientAddr, not the authenticated caller
                continue
            }
            if signer.Weight >= permission.Threshold &&
                permission.CheckPermissionGrantedForContracts(transaction.TXContract_UpdateAccountPermissionContractType) {
                return true
            }
        }
    }
    return false
}

(c) The dispatch makes RecipientAddr attacker-controlled — kvm/vmhost/vmhooks/baseOps.go:2464 prepareIndirectContractCallInput (invoked when a contract calls the built-in via ExecuteOnDestContext):

contractCallInput := &vmcommon.ContractCallInput{
    VMInput: vmcommon.VMInput{
        CallerAddr: sender,        // the calling contract (authenticated) — NOT used by the handler
        Arguments:  data,          // attacker-chosen: [V, attackerPermissions]
        ...
    },
    RecipientAddr: destination,    // the contract's chosen `dest` argument — attacker sets this to V
    Function:      string(function),
}

(d) Every account with configured permissions is its own signer — core/kapp/accounts/accounts.go:1848 createDefaultOwnerPermission (appended by UpdatePermission when no Owner permission is supplied):

return &state.Permission{
    Type:      state.Permission_Owner,   // Owner grants ALL contract types incl. type 22
    Threshold: 1,
    Signers: []*state.Key{
        { Address: ownerAcc.AddressBytes(), Weight: 1 },   // the account signs for itself
    },
}

So contractHasValidPermission(V.perms, RecipientAddr=V) finds V's own address as a Weight 1 >= Threshold 1 Owner signer → returns true.

(e) Contrast — the sibling handler does it correctly — core/kapp/builtInFunctions/kleverChangeOwnerAddress.go:86:

callerAddress := vmInput.CallerAddr                        // authenticated caller
...
if !bytes.Equal(callerAddress, acc.GetOwnerAddress()) {    // checks the CALLER, not RecipientAddr
    return nil, ErrOperationNotPermitted
}

Putting it together — the attacker's contract call:

ExecuteOnDestContext(
    gas, dest = V,                       // → RecipientAddr = V
    value = 0,
    function = "KleverUpdateAccountPermission",
    args = [ V, attackerOwnerPermsWithOnlyAttackerKey ],   // Arguments[0]=V, Arguments[1]=new perms
)

→ CallerAddr = attackerContract (ignored), RecipientAddr = V, contractHasValidPermission(V.perms, V) == true → V's permissions overwritten with the attacker's key as sole Owner signer. The attacker never held a key of V and provided no signature from V.

POC

put the following poc testcase under /core/kapp/builtInFunctions/

POC Code: https://gist.github.com/mabdullah22/a41f90aa5ba86bbebf121f739bd5f5e9

Run:

cd klever-go
GOTOOLCHAIN=auto go test ./core/kapp/builtInFunctions/ -run TestPoC_PermTakeover -v

Output:

TAKEOVER CONFIRMED: caller="attacker-contract" (attacker SC) rewrote account V="victim-account-V"; new sole owner signer="attacker-key-EVIL"
--- PASS: TestPoC_PermTakeover
--- PASS: TestPoC_PermTakeover_NoStoredPermsIsSafe

The harm asserted is the takeover itself: after the call, V's permission set is a single Owner permission whose sole signer is the attacker's key; V's original owner signer is gone.

Impact

Full takeover of any account that has configured permissions i.e. every multisig / advanced-permission account , by an attacker who deploys a cheap smart contract and supplies only public on-chain addresses (no keys, no signatures from the victim). After takeover the attacker controls all of the victim's operations → theft or permanent lock of all the account's assets. Reachable via a permissionlessly-deployed contract (the plain-tx path is safe, so it is Critical-via-contract, not fully no-contract). No fork flag gates it.

Severity Critical: Impact High (full account/asset compromise)

Recommendation

Authorize against the authenticated caller, mirroring kleverChangeOwnerAddress:

if !e.contractHasValidPermission(acc.GetPermissions(), vmInput.CallerAddr) { ... }

Reconcile the SC-call authority model: on the built-in path CallerAddr is the calling contract, so a contract should only be able to update permissions of accounts that legitimately list it as an authorized signer — never an arbitrary victim. Consider also requiring the target account (Arguments[0]) to equal the authorized caller's account, matching the native tx.GetSender() model.