From b5c0b04263cd7bb03b78692611f3b26f881672a1 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 14 Nov 2024 12:00:26 +0700 Subject: [PATCH 1/3] fix: remediations. allow receive eth and withdraw stuck eth --- .../common/BiconomyTokenPaymasterErrors.sol | 10 ++++++++++ contracts/token/BiconomyTokenPaymaster.sol | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/contracts/common/BiconomyTokenPaymasterErrors.sol b/contracts/common/BiconomyTokenPaymasterErrors.sol index b77a63f..6188f11 100644 --- a/contracts/common/BiconomyTokenPaymasterErrors.sol +++ b/contracts/common/BiconomyTokenPaymasterErrors.sol @@ -75,4 +75,14 @@ contract BiconomyTokenPaymasterErrors { * @notice Throws when external signer's signature has invalid length */ error InvalidSignatureLength(); + + /** + * @notice Throws when ETH withdrawal fails + */ + error WithdrawalFailed(); + + /** + * @notice Emitted when ETH is withdrawn from the paymaster + */ + event EthWithdrawn(address indexed recipient, uint256 indexed amount); } diff --git a/contracts/token/BiconomyTokenPaymaster.sol b/contracts/token/BiconomyTokenPaymaster.sol index 75b5726..1642099 100644 --- a/contracts/token/BiconomyTokenPaymaster.sol +++ b/contracts/token/BiconomyTokenPaymaster.sol @@ -133,6 +133,10 @@ contract BiconomyTokenPaymaster is } } + receive() external payable { + // no need to emit an event here + } + /** * @dev pull tokens out of paymaster in case they were sent to the paymaster at any point. * @param token the token deposit to withdraw @@ -143,6 +147,19 @@ contract BiconomyTokenPaymaster is _withdrawERC20(token, target, amount); } + /** + * @dev Withdraw ETH from the paymaster + * @param recipient The address to send the ETH to + * @param amount The amount of ETH to withdraw + */ + function withdrawEth(address payable recipient, uint256 amount) external payable onlyOwner nonReentrant { + (bool success,) = recipient.call{ value: amount }(""); + if (!success) { + revert WithdrawalFailed(); + } + emit EthWithdrawn(recipient, amount); + } + /** * @dev pull tokens out of paymaster in case they were sent to the paymaster at any point. * @param token the token deposit to withdraw From c8943971f852acb555493f627ed95ffaf9185ebe Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 14 Nov 2024 14:45:05 +0700 Subject: [PATCH 2/3] fix: remediations use SafeERC20.forceApprove --- contracts/token/BiconomyTokenPaymaster.sol | 3 ++- contracts/token/swaps/Uniswapper.sol | 3 ++- package.json | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/token/BiconomyTokenPaymaster.sol b/contracts/token/BiconomyTokenPaymaster.sol index 1642099..53e7cad 100644 --- a/contracts/token/BiconomyTokenPaymaster.sol +++ b/contracts/token/BiconomyTokenPaymaster.sol @@ -5,6 +5,7 @@ import { ReentrancyGuardTransient } from "@openzeppelin/contracts/utils/Reentran import { IEntryPoint } from "account-abstraction/interfaces/IEntryPoint.sol"; import { PackedUserOperation, UserOperationLib } from "account-abstraction/core/UserOperationLib.sol"; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import { SafeTransferLib } from "solady/utils/SafeTransferLib.sol"; import { BasePaymaster } from "../base/BasePaymaster.sol"; @@ -129,7 +130,7 @@ contract BiconomyTokenPaymaster is // Approve swappable tokens for max amount uint256 length = swappableTokens.length; for (uint256 i; i < length; i++) { - IERC20(swappableTokens[i]).approve(address(uniswapRouterArg), type(uint256).max); + SafeERC20.forceApprove(IERC20(swappableTokens[i]), address(uniswapRouterArg), type(uint256).max); } } diff --git a/contracts/token/swaps/Uniswapper.sol b/contracts/token/swaps/Uniswapper.sol index 2994478..9671ec8 100644 --- a/contracts/token/swaps/Uniswapper.sol +++ b/contracts/token/swaps/Uniswapper.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.27; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@uniswap/v3-periphery/contracts/interfaces/ISwapRouter.sol"; import "@uniswap/v3-periphery/contracts/interfaces/IPeripheryPayments.sol"; @@ -48,7 +49,7 @@ abstract contract Uniswapper { } function _setTokenPool(address token, uint24 poolFeeTier) internal { - IERC20(token).approve(address(uniswapRouter), type(uint256).max); // one time max approval + SafeERC20.forceApprove(IERC20(token), address(uniswapRouter), type(uint256).max); // one time max approval tokenToPools[token] = poolFeeTier; // set mapping of token to uniswap pool to use for swap } diff --git a/package.json b/package.json index fd27a7d..96bcd82 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "url": "https://github.com/bcnmy" }, "dependencies": { - "@openzeppelin/contracts": "5.0.2", + "@openzeppelin/contracts": "5.1.0", "@rhinestone/modulekit": "^0.4.10", "@uniswap/v3-core": "https://github.com/Uniswap/v3-core#0.8", "@uniswap/v3-periphery": "https://github.com/Uniswap/v3-periphery#0.8", From 8818f487604651c79640d8fdfa1c00f1de6a386d Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:03:37 +0700 Subject: [PATCH 3/3] fix: remediation apply price markup to maxpenalty factor --- contracts/token/BiconomyTokenPaymaster.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/BiconomyTokenPaymaster.sol b/contracts/token/BiconomyTokenPaymaster.sol index 75b5726..44c8f56 100644 --- a/contracts/token/BiconomyTokenPaymaster.sol +++ b/contracts/token/BiconomyTokenPaymaster.sol @@ -493,7 +493,7 @@ contract BiconomyTokenPaymaster is // deduct max penalty from the token amount we pass to the postOp // so we don't refund it at postOp - context = abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/1e18), tokenPrice, externalPriceMarkup, userOpHash); + context = abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice*externalPriceMarkup)/(1e18*_PRICE_DENOMINATOR)), tokenPrice, externalPriceMarkup, userOpHash); validationData = _packValidationData(false, validUntil, validAfter); } else if (mode == PaymasterMode.INDEPENDENT) { // Use only oracles for the token specified in modeSpecificData @@ -518,7 +518,7 @@ contract BiconomyTokenPaymaster is SafeTransferLib.safeTransferFrom(tokenAddress, userOp.sender, address(this), tokenAmount); context = - abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/1e18), tokenPrice, independentPriceMarkup, userOpHash); + abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice*independentPriceMarkup)/(1e18*_PRICE_DENOMINATOR)), tokenPrice, independentPriceMarkup, userOpHash); validationData = 0; // Validation success and price is valid indefinetly } }