# KINTO FINDINGS
### 2.1 Critical
### 2.2 High
#### 1. SponsorPaymaster owner should be restricted to withdraw
##### Description
`SponsorPaymaster._postOp()` has these lines:
```
balances[account] -= ethCost;
contractSpent[account] += ethCost;
balances[owner()] += ethCost;
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L160-L162
So, the sum of balances is not changed during UserOp execution.
The same sum of user balances is duplicated in EntryPoint.deposits[], as every SponsorPaymaster deposit and withdrawal is duplicated the same way towards the EntryPoint.
But every UserOp execution decreases the sum of balances stored in the EntryPoint (written ETH balance stored in `Entrypoint.deposits[paymaster]` is transferred to a `beneficiary`). But the sum of balances written in Paymaster for all accounts has not changed.
It means that some of the `SponsorPaymaster` accounts will not be able to withdraw.
Owner accumulates `ethCost` on every `SponsorPaymaster` usage. If the owner withdraws using `SponsorPaymaster.withdrawTokensTo()`, some other of the `SponsorPaymaster` depositors will have fewer funds actually stored at `Entrypoint`.
As a result, operations will fail - having a zero balance on EntryPoint, but a non-zero balance on `SponsorPaymaster`.
##### Recommendation
We recommend removing `balances[owner()] += ethCost;` from `SponsorPaymaster._postOp()`.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 2. Creating KintoWallet that is protected from sanctions and KYC missed
##### Description
Imagine the following attack:
1) An attacker finds a very trusted KYCed user (trustedUserA)
2) The attacker creates a `KintoWallet`, where `owner=trustedUserA` and `recoverer=Attacker`.
Only owner is checked, the Attacker is not checked for KYC.
3) For this new KintoWallet Attacker calls `startRecovery()`
4) In 7 days Attacker calls `KintoWallet.finishRecovery()`, with the following set of owners inputted:
owner[0] = trustedUserA, will check KYC again
owner[1] = Attacker, no KYC check
owner[2] = Attacker EOA 2, no KYC check
5) Attacker uses Entrypoint to call `KintoWallet.setSignerPolicy(2)`, which means N-1 required signers. This new transaction will be easy to sign, as by default `signerPolicy` is 1 (any of owners can sign)
As a result:
- owner[0] is a trusted user, who has KYC, so the KintoWallet will pass KYC checks
- owner[0] alone will not be able to sign transactions, as 2 signers are required
- Attacker can use 2 accounts to sign transactions from this Wallet.
- KintoID will be required to revoke KYC from a trusted owner[0] who is just a user who made nothing wrong and don't even know that such wallet exists.
https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWallet.sol#L204
The same attack can be done by the following steps:
1) Attacker passes KYC and creates KintoWallet where owner=Attacker
2) Attacker calls resetSigners with the same set of owners
3) If KYC is revoked from the attacker, the attacker still has the majority 2/3 signs and can use the wallet (same result)
##### Recommendation
Consider the following fixes:
- requiring KYC from msg.sender in `createAccount()`
- do not accept an owner signature if it does not pass isKYC(), check KYC for every owner
- check KYCs for a new set of owners
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 3. Draining SponsorPaymaster.balances with spam operations
##### Description
Currently, users are not charged for their operations.
Gas compensations are taken from `SponsorPaymaster.balances[account]`, where `account` is the destination address in `KintoWallet.execute()` or `KintoWallet.executeBatch()`.
These accounts are charged with their `SponsorPaymaster.balances[]`.
As a result, users can spam multiple `UserOps`, and gas costs will be compensated from these account balances. This attack has no cost, but results in losses for `account` if actively used.
Moreover, these `accounts` can be another `KintoWallets` if they have some balance on `SponsorPaymaster.balances[]`.
An attacker should build a `calldata` where the target address is another KintoWallet and the function signature is some read-only function on the target `KintoWallet`.
##### Recommendation
The key problem here is that the attack has no cost and the `UserOp.account` does not pay for operations.
We recommend charging `UserOp.account` and introducing some cost for potential attackers.
##### Status
**NEW**
##### Client's commentary
> Comment here
### 2.3 Medium
#### 1. SponsorPaymaster not compatible with KintoWallet.executeBatch()
##### Description
`KintoWallet.executeBatch()` make multiple calls to a list of `address[] calldata dest`
But SponsorPaymaster strictly chooses the first input address as a sponsor for gas costs, and it is only one address.
```
address targetAccount = address(bytes20(userOp.callData[16:]));
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L142
This logic does not work for KintoWallet.executeBatch() which accept an array as the first argument:
```
function executeBatch(address[] calldata dest, uint256[] calldata values, bytes[] calldata func) external override {
_requireFromEntryPoint();
require(dest.length == func.length, 'wrong array lengths');
for (uint256 i = 0; i < dest.length; i++) {
dest[i].functionCallWithValue(func[i], values[i]);
}
}
```
So the UserOp.calldata cannot be properly decoded to extract dest addresses, as the calldata for array arguments do not start with the address.
- Calldata of one address element:
`0xc79fb9f5000000000000000000000000a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48`
- Calldata of one address element as an array: `0x4ab285f400000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48`
Moreover, the array is designed to have a few addressed, usually different addresses. It means it is not correct to choose only one address, even if the decoding code is adjusted.
##### Recommendation
The current logic of choosing the `targetAccount` cannot work with the `executeBatch()`. We recommend either removing `executeBatch()` or reworking SponsorPaymaster to separately measure the gas cost for every `dest` in the inputted array.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 2. KintoWallet signatures underflow
##### Description
KintoWallet._validateSignatures has the following lines:
```
...
if (owners.length == 2) {
(signatures[0], signatures[1]) = ByteSignature.extractTwoSignatures(userOp.signature);
} else {
(signatures[0], signatures[1], signatures[2]) = ByteSignature.extractThreeSignatures(userOp.signature);
}
for (uint i = 0; i < owners.length; i++) {
if (owners[i] == hash.recover(signatures[i])) {
requiredSigners--;
}
}
return requiredSigners;
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWallet.sol#L220-L230
Imagine we have 3 owners, and signerPolicy=2.
It means that at least N-1 correct signatures are required = at least 2 sigantures.
Every correct signature found in the loop will decrease `requiredSigners`.
If `requiredSigners` is equal to 0 in the end, it means that the validation went correctly.
But the third signature will make the underflow, and the whole validation will fail.
##### Recommendation
Consider exiting the loop when `requiredSigners` reaches 0.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 3. KintoID.lastMonitoredAt fails isKYC() for all accounts if no new monitors
##### Description
`KintoID.lastMonitoredAt` is a very risky parameter. It is reset to the current `block.timestamp` every time `monitor()` is called.
If `monitor()` is not called for 7 days - all `isKYC()` checks for all accounts will return `false`.
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L282
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L300
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L291
Being KYCed is extremely important to make transactions. So `lastMonitoredAt` can stop the whole chain activity.
but `lastMonitoredAt` can become outdated if new treats and sanctions are set via other public functions: `addTrait()`, `removeTrait()`, `addSanction()`, `removeSanction()`
##### Recommendation
We recommend updating `lastMonitoredAt` in `addTrait()`, `removeTrait()`, `addSanction()`, `removeSanction()`
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 4. onlySignerVerified() usage - nonce mistake
##### Description
`onlySignerVerified()` is used in two contracts - `Faucet` and `KintoID`.
Internally this function checks nonces for a `_signature.signer`.
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L371-L398
So it means, that the nonce for this `_signature.signer` should be incremented.
But contracts do mistake and increments nonce for `_signatureData.account`. As a result, a signatures can be used twice when calling functions `Faucet.claimOnBehalf()`, `KintoID.mintIndividualKyc()` and `KintoID.mintCompanyKyc()`.
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L178
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L154
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/Faucet.sol#L48
##### Recommendation
We recommend incrementing nonce for `_signature.signer`.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 5. KintoWalletFactory.deployContract() wrong payable deploy
##### Description
`KintoWalletFactory` has `deployContract()` function:
```
function deployContract(
uint amount,
bytes memory bytecode,
bytes32 salt
) external override returns (address) {
require(kintoID.isKYC(msg.sender), 'KYC required');
return Create2.deploy(amount, salt, bytecode);
}
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWalletFactory.sol#L130-L137
This function accepts the `amount` parameter which will be used as a `msg.value` in the payable deployment. But the whole deployContract() is not payable and does not receive the native token.
It is commented as:
```
- the factory must have a balance of at least `amount`.
```
So someone should top up the Factory to have the necessary native balance.
There are multiple problems with this approach:
1) `KintoWalletFactory` does not have functions to receive native balance
2) Even if native balance is deposited, it can be stolen by anyone who is faster to deploy their own smart contracts
##### Recommendation
We recommend having a `payable` modifier in `deployContract()` and checking that the `msg.value==amount`.
##### Status
**NEW**
##### Client's commentary
> Comment here
### 2.4 Low
#### 1. executeBatch() does not check values.length
##### Description
KintoWallet executeBatch() accepts arrays as inputs but checks only that `dest.length == func.length`.
```
function executeBatch(address[] calldata dest, uint256[] calldata values, bytes[] calldata func) external override {
_requireFromEntryPoint();
require(dest.length == func.length, 'wrong array lengths');
for (uint256 i = 0; i < dest.length; i++) {
dest[i].functionCallWithValue(func[i], values[i]);
}
}
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWallet.sol#L112-L118
In fact, checking `values.length` is also necessary, because it should be exactly the same length as `dest` and `func`. It can be an array of zero values. Otherwise, it can be reverted, if e.g. `values[i]` tries to take the index above the length.
##### Recommendation
We recommend additionally checking that `dest.length == values.length`.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 2. KintoWallet.onlyFactory not used
##### Description
KintoWallet has the following modifier:
```
modifier onlyFactory() {
_onlyFactory();
_;
}
```
But it is never used.
Moreover, `factory` address is never set, so it remain `address(0)`.
##### Recommendation
We recommend setting the `factory` address, implementing new logic using `onlyFactory`. If it is not required, it can be deleted.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 3. SponsorPaymaster unlocks for the deployer, not necessarily the owner
##### Description
`SponsorPaymaster.initialize()` calls internally `unlockTokenDeposit()`, which sets:
```
unlockBlock[msg.sender] = block.number;
```
It likely aims to unlock withdrawals for the owner.
There is the comment proving this intend.
```
...
// unlocks owner
...
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L48-L49
But it unlocks not the `owner`, but the `msg.sender` - the deployer.
So if `initialize()` indicates the owner which is different from the deployer, only the deployer will be unlocked, not the owner.
##### Recommendation
We recommend replacing `unlockTokenDeposit();` with the direct line `unlockBlock[_owner] = block.number;` if it was designed to unlock the owner and not the deployer.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 4. KintoID, Factory and SponsorPaymaster initialize frontrun
##### Description
KintoId, Factory and SponsorPaymaster contracts require calling `initialize()` during the deployment. These functions are public. So they can be frontrun by not authorized addresses.
In the Factory contract, it will allow to set own KintoId address, in KintoID it will allow to set malicious roles, and in SponsorPaymaster it will allow to set the owner which will receive gas compensation balance.
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L63-L73
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWalletFactory.sol#L50-L56
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L45-L50
The severity of this issue is high enough but it must be noted that the likelihood is quite low as the deployment process happens in the controlled chain.
##### Recommendation
We recommend having either all deployment steps in one transaction or setting the allowed `initialize()` caller in the `constructor()`.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 5. KintoID.monitor() checks KYC but other functions not
##### Description
KintoID.monitor() function checks KYC for every address inputted:
```
require(balanceOf(_accounts[i], 1) > 0, 'Invalid account address');
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/KintoID.sol#L195
But other functions that do the same operations do not check KYC:
- addTrait()
- removeTrait()
- addSanction()
- removeSanction()
Moreover, KYC can be revoked (NFT burn) and given again (NFT mint).
All traits and sanctions remain unchanged in the meta.
So in fact traits and sanctions can exist even for accounts with no KYC.
##### Recommendation
The simplest solution is removing the requirement for an address KYC in `KintoID.monitor()`. Otherwise, consider adding this check to other trait and sanction functions and deleting the meta storage for burned NFTs.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 6. upgradeAllWalletImplementations() does not check new != old
##### Description
`KintoWalletFactory` has `upgradeAllWalletImplementations()`.
```
function upgradeAllWalletImplementations(
IKintoWallet newImplementationWallet
) external override {
require(msg.sender == factoryOwner, 'only owner');
require(address(newImplementationWallet) != address(0), 'invalid address');
factoryWalletVersion++;
emit KintoWalletFactoryUpgraded(address(newImplementationWallet),
address(newImplementationWallet));
beacon.upgradeTo(address(newImplementationWallet));
}
```
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/wallet/KintoWalletFactory.sol#L62-L71
But it does not check that `newImplementationWallet` is not equal to the old implementation (`UpgradableBeacon._implementation`).
##### Recommendation
Consider implementing this check.
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 7. SponsorPaymaster.deposit() does not deposit for any user
##### Description
`SponsorPaymaster` inherits from the `BasePaymaster` which has the `deposit()` function. It just uses the provided `msg.value` and deposits it to the `EntryPoint`.
- https://github.com/eth-infinitism/account-abstraction/blob/558e024161cf355e5a0fa59b1b5c0c5cb5aeac74/contracts/core/BasePaymaster.sol#L81-L83
Balance for the `msg.sender` is not increased.
It is misleading, given that `SponsorPaymaster` has `addDepositFor()` which correctly increases the balance for an address.
`deposit()` is used only internally as `this.deposit()` to transfer funds to the `Entrypoint` during the `addDepositFor()`.
It is better to remove this function. It can be an argument that this function can be used to donate native tokens. But there are better ways - using `addDepositFor()`.
##### Recommendation
Consider overriding `deposit()` function requiring that `msg.sender==address(this)`.
A good reference for the same situation is the EntryPoint:
https://github.com/eth-infinitism/account-abstraction/blob/558e024161cf355e5a0fa59b1b5c0c5cb5aeac74/contracts/core/EntryPoint.sol#L265
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 8. Faucet.withdrawAll() does not change active status
##### Description
`Faucet.active` is `false` if the native balance is below `CLAIM_AMOUNT`.
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/Faucet.sol#L78-L79
But when `withdrawAll()` is called it does not change the status:
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/Faucet.sol#L54-L56
##### Recommendation
We recommend setting `active` as `false` in `withdrawAll()`
##### Status
**NEW**
##### Client's commentary
> Comment here
#### 9. Read-only reentrancy possible in SponsorPaymaster
##### Description
SponsorPaymaster.withdrawTokensTo() does not follow CEI pattern:
1) sends native tokens to the withdrawal target
2) update balances
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L107-L108
For example, in this case the withdrawal target is able to receive tokens and run addition logic.
At least, in this reentrancy, some read-only functions will return wrong balances:
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L119-L120
- https://github.com/KintoXYZ/kinto-core/blob/f7dd98f66b9dfba1f73758703b808051196e740b/src/paymasters/SponsorPaymaster.sol#L32
##### Recommendation
We recommend replacing lines from:
```
withdrawTo(payable(target), amount);
balances[msg.sender] -= amount;
```
...to:
```
balances[msg.sender] -= amount;
withdrawTo(payable(target), amount);
```
##### Status
**NEW**
##### Client's commentary
> Comment here
Google Drive
Gist
Clipboard
Sign in with Wallet
