Kwenta A-16

When executing the postOp() logic, the MarginPaymaster contract will attempt to pull assets from the user’s withdrawal margin if he does not have available USDC tokens. To handle all potential revert scenarios, the withdrawFromMargin() function checks that the proper accountId is used and that the paymaster contract has the *PERPS_MODIFY_COLLATERAL_PERMISSION* permission. After access control checks, the available withdrawal margin is checked and function execution returns if negative.

int256 withdrawableMargin = perpsMarketSNXV3.getWithdrawableMargin(
    accountId
);

if (withdrawableMargin < 0) return 0;

Reference: MarginPaymaster.sol#L392-396

However, if the account's available margin is zero, the function will not return, and it will attempt to execute a modifyCollateral() call with a zero amount:

uint256 withdrawableMarginUint = uint256(withdrawableMargin);

uint256 amountToPullFromMargin = min(
    sUSDToWithdrawFromMargin,
    withdrawableMarginUint
);

// pull sUSD from margin
perpsMarketSNXV3.modifyCollateral(
    accountId,
    sUSDId,
    -int256(amountToPullFromMargin)
);

Reference: MarginPaymaster.sol#L397-409

This will cause the perpsMarketSNXV3.modifyCollateral() to revert with an InvalidAmountDelta error, causing the postOp() to revert.

Remediations to Consider:

Consider returning 0 if the available withdrawal margin is ≤ 0 when verifying the withdrawableMargin value.

In the MarginPaymaster contract, the function withdrawFromMargin() is responsible for recouping the transaction cost from the associated account’s collateral. When retrieving the owner of the corresponding account fails, it falls back to functionality that tries to withdraw assets from the first account associated with a particular user.

if (accountId != 0) {
    // check if the account Id is valid
    try snxV3AccountsModule.ownerOf(accountId) returns (address owner) {
        // only allow the owners accounts to subsidise gas
        if (owner != sender) return 0;
    } catch {
        // set accountId to zero, and then check if the sender has an account on-chain
        accountId = 0;
    }
}

Reference: MarginPaymaster.sol#L366-375

However, when retrieving the owner of the provided account does not fail, but instead returns a different owner, execution returns immediately instead and does not try to withdraw assets from the first account associated with a particular user.

As a result, if the caller provides the account of a different user, they will never be charged gas transaction costs. Instead, MarginPaymaster will be incorrectly sponsoring these transactions.

Remediations to Consider

• Make the following change in if branch

  - if (owner != sender) return 0;
  + if (owner != sender) accountId = 0;
  

  Reference: MarginPaymaster.sol#L370

In the MarginPaymaster contract, the owner may set and update the percentageMarkup value using setPercentageMarkup(). Currently, there is no input value restriction, and high values for percentageMarkup will result in charging the user more than expected.

Therefore, if a high percentageMarkup value is set accidentally or intentionally by a compromised contract owner, it may drain the smart account’s balance and free-to-withdraw margin from an associated account on the SNXV3 perp market.

Remediations to Consider

• Add restrictions and corresponding checks on percentageMarkup value initialization and updates.

Several important state-changing functions do not emit events.

• setPercentageMarkup()
• setAuthorizer()

Remediations to Consider

• Add corresponding events for easier off-chain tracking and monitoring.

The getHash() function uses both paymasterAddress and address(this) as hash inputs. However, these two variables should have the same value, making the encoding of both redundant.

    return
        keccak256(
            abi.encode(
                ...
                uint256(bytes32(paymasterAddress)),
                ...
                address(this)
            )
        );

Reference: MarginPaymaster.sol#L120-L136

Remediations to Consider

Consider validating that parsed paymasterAddress is equal to address(this) and removing one of those two from the input for hashing.

Currently, to deposit funds to the EntryPoint contract, it is necessary first to send specific amount of native token directly to the MarginPaymaster contract, and then call MarginPaymaster.depositToEntryPoint() function.

However, by adding payable modifier to the depositToEntryPoint() function, funds can be deposited in one transaction.

Consider updating depositToEntryPoint() function signature in the following way:

-    function depositToEntryPoint(uint256 amount) external onlyOwner {
+    function depositToEntryPoint(uint256 amount) payable external onlyOwner {

Reference: MarginPaymaster.sol#L258

Currently, the swapUSDCToETH() function swaps all of the USDC in the MarginPaymaster contract to ETH.

Consider addingusdcAmount as an argument in the swapUSDCToETH() function for more flexible swapping and having less risk, making the Uniswap pool volatile from swapping with the huge amount

Reference: MarginPaymaster.sol#L250

The current MarginPaymaster contract is designed to use the Entrypoint version 0.6.0, released in April 2023. Some major optimizations and changes were deployed with the latest 0.7.0 version, as explicitly described in the release notes, while some were optimizing Paymaster integrations and solving potential issues.

Consider upgrading the Entrypoint version to 0.7.0.

The MarginPaymaster contract inherits the Zap contract, which contains 2 internal functions:

• _zapIn() , and
• _zapOut()

However, MarginPaymaster only uses _zapOut().

Consider removing the _zapIn() function as it is not used.

The MarginPaymaster relies on USDC and Synthetix v3 perpetual market module to calculate and pull assets on the operation sender’s behalf. Both external parties can prevent this contract from working:

• USDC blacklisting the MarginPaymaster address or the sender.
• Synthetix governance disabling the feature flag for the perps market module.

This will essentially cause all UserOp signed with this paymaster as a target to not be executed