Details
### 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`:
```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:
```go
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`):
```go
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):
```go
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`:
```go
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`:
```go
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.
