Skip to content

Commit

Permalink
Merge pull request #39 from bcnmy/fix/remediations-chainlight-BTPMQ4-005
Browse files Browse the repository at this point in the history 
fix: remediations as per discussion
  • Loading branch information
@livingrockrises
livingrockrises authored Nov 20, 2024
2 parents 2439053 + 4e10f3d commit 04580de
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 60 deletions.
5 changes: 5 additions & 0 deletions contracts/common/BiconomyTokenPaymasterErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ contract BiconomyTokenPaymasterErrors {
*/
error InvalidOracleDecimals();

/**
* @notice Throws when price expiry duration is in the past
*/
error InvalidPriceExpiryDuration();

/**
* @notice Throws when external signer's signature has invalid length
*/
Expand Down
14 changes: 9 additions & 5 deletions contracts/interfaces/IBiconomyTokenPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ interface IBiconomyTokenPaymaster {
}

event UpdatedUnaccountedGas(uint256 indexed oldValue, uint256 indexed newValue);
event UpdatedFixedPriceMarkup(uint256 indexed oldValue, uint256 indexed newValue);
event UpdatedFixedPriceMarkup(uint32 indexed oldValue, uint32 indexed newValue);
event UpdatedVerifyingSigner(address indexed oldSigner, address indexed newSigner, address indexed actor);
event UpdatedFeeCollector(address indexed oldFeeCollector, address indexed newFeeCollector, address indexed actor);
event UpdatedPriceExpiryDuration(uint256 indexed oldValue, uint256 indexed newValue);
Expand All @@ -30,23 +30,27 @@ interface IBiconomyTokenPaymaster {
address indexed token,
uint256 nativeCharge,
uint256 tokenCharge,
uint256 priceMarkup,
uint32 priceMarkup,
uint256 tokenPrice,
bytes32 indexed userOpHash
);
event Received(address indexed sender, uint256 value);
event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor);
event UpdatedTokenDirectory(address indexed tokenAddress, IOracle indexed oracle, uint8 decimals);
event AddedToTokenDirectory(address indexed tokenAddress, IOracle indexed oracle, uint8 decimals);
event RemovedFromTokenDirectory(address indexed tokenAddress);
event UpdatedNativeAssetOracle(IOracle indexed oldOracle, IOracle indexed newOracle);
event TokensSwappedAndRefilledEntryPoint(address indexed tokenAddress, uint256 indexed tokenAmount, uint256 indexed amountOut, address actor);
event SwappableTokensAdded(address[] indexed tokenAddresses);

function setSigner(address newVerifyingSigner) external payable;

function setUnaccountedGas(uint256 value) external payable;

function setPriceMarkup(uint256 newUnaccountedGas) external payable;
function setPriceMarkup(uint32 newPriceMarkup) external payable;

function setPriceExpiryDuration(uint256 newPriceExpiryDuration) external payable;

function setNativeAssetToUsdOracle(IOracle oracle) external payable;

function updateTokenDirectory(address tokenAddress, IOracle oracle) external payable;
function addToTokenDirectory(address tokenAddress, IOracle oracle) external payable;
}
8 changes: 4 additions & 4 deletions contracts/libraries/TokenPaymasterParserLib.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,17 @@ library TokenPaymasterParserLib {
uint48 validUntil,
uint48 validAfter,
address tokenAddress,
uint128 tokenPrice, // Review: why uint128 and not uint256. in independent mode it is uint256
uint256 tokenPrice, // Review: why uint128 and not uint256. in independent mode it is uint256
uint32 externalPriceMarkup,
bytes memory signature
)
{
validUntil = uint48(bytes6(modeSpecificData[:6]));
validAfter = uint48(bytes6(modeSpecificData[6:12]));
tokenAddress = address(bytes20(modeSpecificData[12:32]));
tokenPrice = uint128(bytes16(modeSpecificData[32:48]));
externalPriceMarkup = uint32(bytes4(modeSpecificData[48:52]));
signature = modeSpecificData[52:];
tokenPrice = uint256(bytes32(modeSpecificData[32:64]));
externalPriceMarkup = uint32(bytes4(modeSpecificData[64:68]));
signature = modeSpecificData[68:];
}

function parseIndependentModeSpecificData(
Expand Down
91 changes: 50 additions & 41 deletions contracts/token/BiconomyTokenPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,30 +52,25 @@ contract BiconomyTokenPaymaster is
// State variables
address public verifyingSigner; // entity used to provide external token price and markup
uint256 public unaccountedGas;
uint256 public independentPriceMarkup; // price markup used for independent mode
uint32 public independentPriceMarkup; // price markup used for independent mode
uint256 public priceExpiryDuration; // oracle price expiry duration
IOracle public nativeAssetToUsdOracle; // ETH -> USD price oracle
mapping(address => TokenInfo) public independentTokenDirectory; // mapping of token address => info for tokens
// supported in // independent mode

// PAYMASTER_ID_OFFSET
// Note: Temp
uint256 private constant _UNACCOUNTED_GAS_LIMIT = 200_000; // Limit for unaccounted gas cost
uint256 private constant _PRICE_DENOMINATOR = 1e6; // Denominator used when calculating cost with price markup
uint256 private constant _MAX_PRICE_MARKUP = 2e6; // 100% premium on price (2e6/PRICE_DENOMINATOR)

// Note: _priceExpiryDuration is common for all the feeds.
// Note: _independentPriceMarkup is common for all the independent tokens.
// Todo: add cases when uniswap is not available
// Note: swapTokenAndDeposit: we may not need to keep this onlyOwner
uint32 private constant _PRICE_DENOMINATOR = 1e6; // Denominator used when calculating cost with price markup
uint32 private constant _MAX_PRICE_MARKUP = 2e6; // 100% premium on price (2e6/PRICE_DENOMINATOR)
uint256 private immutable _NATIVE_TOKEN_DECIMALS;

constructor(
address owner,
address verifyingSignerArg,
IEntryPoint entryPoint,
uint256 unaccountedGasArg,
uint256 independentPriceMarkupArg, // price markup used for independent mode
uint32 independentPriceMarkupArg, // price markup used for independent mode
uint256 priceExpiryDurationArg,
uint256 nativeAssetDecimalsArg,
IOracle nativeAssetToUsdOracleArg,
ISwapRouter uniswapRouterArg,
address wrappedNativeArg,
Expand All @@ -88,6 +83,7 @@ contract BiconomyTokenPaymaster is
BasePaymaster(owner, entryPoint)
Uniswapper(uniswapRouterArg, wrappedNativeArg, swappableTokens, swappableTokenPoolFeeTiers)
{
_NATIVE_TOKEN_DECIMALS = nativeAssetDecimalsArg;
if (_isContract(verifyingSignerArg)) {
revert VerifyingSignerCanNotBeContract();
}
Expand All @@ -108,6 +104,9 @@ contract BiconomyTokenPaymaster is
// ETH -> USD will always have 8 decimals for Chainlink and TWAP
revert InvalidOracleDecimals();
}
if (block.timestamp < priceExpiryDurationArg) {
revert InvalidPriceExpiryDuration();
}

// Set state variables
assembly ("memory-safe") {
Expand All @@ -127,11 +126,6 @@ contract BiconomyTokenPaymaster is
independentTokenDirectory[independentTokensArg[i]] =
TokenInfo(oraclesArg[i], 10 ** IERC20Metadata(independentTokensArg[i]).decimals());
}
// Approve swappable tokens for max amount
uint256 length = swappableTokens.length;
for (uint256 i; i < length; i++) {
SafeERC20.forceApprove(IERC20(swappableTokens[i]), address(uniswapRouterArg), type(uint256).max);
}
}

receive() external payable {
Expand Down Expand Up @@ -225,8 +219,8 @@ contract BiconomyTokenPaymaster is
* @dev Set a new verifying signer address.
* Can only be called by the owner of the contract.
* @param newVerifyingSigner The new address to be set as the verifying signer.
* @notice If _newVerifyingSigner is set to zero address, it will revert with an error.
* After setting the new signer address, it will emit an event VerifyingSignerChanged.
* @notice If newVerifyingSigner is set to zero address, it will revert with an error.
* After setting the new signer address, it will emit an event UpdatedVerifyingSigner.
*/
function setSigner(address newVerifyingSigner) external payable onlyOwner {
if (_isContract(newVerifyingSigner)) revert VerifyingSignerCanNotBeContract();
Expand Down Expand Up @@ -261,24 +255,25 @@ contract BiconomyTokenPaymaster is
* @param newIndependentPriceMarkup The new value to be set as the price markup
* @notice only to be called by the owner of the contract.
*/
function setPriceMarkup(uint256 newIndependentPriceMarkup) external payable onlyOwner {
function setPriceMarkup(uint32 newIndependentPriceMarkup) external payable onlyOwner {
if (newIndependentPriceMarkup > _MAX_PRICE_MARKUP || newIndependentPriceMarkup < _PRICE_DENOMINATOR) {
// Not between 0% and 100% markup
revert InvalidPriceMarkup();
}
uint256 oldIndependentPriceMarkup = independentPriceMarkup;
uint32 oldIndependentPriceMarkup = independentPriceMarkup;
assembly ("memory-safe") {
sstore(independentPriceMarkup.slot, newIndependentPriceMarkup)
}
emit UpdatedFixedPriceMarkup(oldIndependentPriceMarkup, newIndependentPriceMarkup);
}

/**
* @dev Set a new priceMarkup value.
* @param newPriceExpiryDuration The new value to be set as the unaccounted gas value
* @dev Set a new price expiry duration.
* @param newPriceExpiryDuration The new value to be set as the price expiry duration
* @notice only to be called by the owner of the contract.
*/
function setPriceExpiryDuration(uint256 newPriceExpiryDuration) external payable onlyOwner {
if(block.timestamp < newPriceExpiryDuration) revert InvalidPriceExpiryDuration();
uint256 oldPriceExpiryDuration = priceExpiryDuration;
assembly ("memory-safe") {
sstore(priceExpiryDuration.slot, newPriceExpiryDuration)
Expand Down Expand Up @@ -311,7 +306,7 @@ contract BiconomyTokenPaymaster is
* @param oracle The oracle to use for the specified token
* @notice only to be called by the owner of the contract.
*/
function updateTokenDirectory(address tokenAddress, IOracle oracle) external payable onlyOwner {
function addToTokenDirectory(address tokenAddress, IOracle oracle) external payable onlyOwner {
if (oracle.decimals() != 8) {
// Token -> USD will always have 8 decimals
revert InvalidOracleDecimals();
Expand All @@ -320,7 +315,17 @@ contract BiconomyTokenPaymaster is
uint8 decimals = IERC20Metadata(tokenAddress).decimals();
independentTokenDirectory[tokenAddress] = TokenInfo(oracle, 10 ** decimals);

emit UpdatedTokenDirectory(tokenAddress, oracle, decimals);
emit AddedToTokenDirectory(tokenAddress, oracle, decimals);
}

/**
* @dev Remove a token from the independentTokenDirectory mapping.
* @param tokenAddress The token address to remove from directory
* @notice only to be called by the owner of the contract.
*/
function removeFromTokenDirectory(address tokenAddress) external payable onlyOwner {
delete independentTokenDirectory[tokenAddress];
emit RemovedFromTokenDirectory(tokenAddress );
}

/**
Expand All @@ -344,6 +349,7 @@ contract BiconomyTokenPaymaster is
for (uint256 i = 0; i < tokenAddresses.length; ++i) {
_setTokenPool(tokenAddresses[i], poolFeeTiers[i]);
}
emit SwappableTokensAdded(tokenAddresses);
}

/**
Expand All @@ -360,7 +366,7 @@ contract BiconomyTokenPaymaster is
)
external
payable
onlyOwner
nonReentrant
{
// Swap tokens for WETH
uint256 amountOut = _swapTokenToWeth(tokenAddress, tokenAmount, minEthAmountRecevied);
Expand All @@ -370,6 +376,7 @@ contract BiconomyTokenPaymaster is
// Deposit ETH into EP
entryPoint.depositTo{ value: amountOut }(address(this));
}
emit TokensSwappedAndRefilledEntryPoint(tokenAddress, tokenAmount, amountOut, msg.sender);
}

/**
Expand Down Expand Up @@ -403,7 +410,7 @@ contract BiconomyTokenPaymaster is
uint48 validUntil,
uint48 validAfter,
address tokenAddress,
uint128 tokenPrice,
uint256 tokenPrice,
uint32 externalPriceMarkup
)
public
Expand Down Expand Up @@ -471,8 +478,7 @@ contract BiconomyTokenPaymaster is
uint48 validUntil,
uint48 validAfter,
address tokenAddress,
// Review if uint128 is enough
uint128 tokenPrice, // NotE: what backend should pass is token/native * 10^token decimals
uint256 tokenPrice, // NotE: what backend should pass is token/native * 10^token decimals
uint32 externalPriceMarkup,
bytes memory signature
) = modeSpecificData.parseExternalModeSpecificData();
Expand Down Expand Up @@ -503,15 +509,15 @@ contract BiconomyTokenPaymaster is
{
uint256 maxFeePerGas = UserOperationLib.unpackMaxFeePerGas(userOp);
tokenAmount = ((maxCost + maxPenalty + (unaccountedGas * maxFeePerGas)) * externalPriceMarkup * tokenPrice)
/ (1e18 * _PRICE_DENOMINATOR);
/ (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
}

// Transfer full amount to this address. Unused amount will be refunded in postOP
SafeTransferLib.safeTransferFrom(tokenAddress, userOp.sender, address(this), tokenAmount);

// 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*externalPriceMarkup)/(1e18*_PRICE_DENOMINATOR)), tokenPrice, externalPriceMarkup, userOpHash);
context = abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice*externalPriceMarkup)/(_NATIVE_TOKEN_DECIMALS*_PRICE_DENOMINATOR)), tokenPrice, externalPriceMarkup, userOpHash);
validationData = _packValidationData(false, validUntil, validAfter);
} else if (mode == PaymasterMode.INDEPENDENT) {
// Use only oracles for the token specified in modeSpecificData
Expand All @@ -522,21 +528,24 @@ contract BiconomyTokenPaymaster is
// Get address for token used to pay
address tokenAddress = modeSpecificData.parseIndependentModeSpecificData();
uint256 tokenPrice = _getPrice(tokenAddress);
if(tokenPrice == 0) {
revert TokenNotSupported();
}
uint256 tokenAmount;

// TODO: Account for penalties here
{
// Calculate token amount to precharge
uint256 maxFeePerGas = UserOperationLib.unpackMaxFeePerGas(userOp);
tokenAmount = ((maxCost + maxPenalty + (unaccountedGas * maxFeePerGas)) * independentPriceMarkup * tokenPrice)
/ (1e18 * _PRICE_DENOMINATOR);
/ (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
}

// Transfer full amount to this address. Unused amount will be refunded in postOP
SafeTransferLib.safeTransferFrom(tokenAddress, userOp.sender, address(this), tokenAmount);

context =
abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice*independentPriceMarkup)/(1e18*_PRICE_DENOMINATOR)), tokenPrice, independentPriceMarkup, userOpHash);
abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice*independentPriceMarkup)/(_NATIVE_TOKEN_DECIMALS*_PRICE_DENOMINATOR)), tokenPrice, independentPriceMarkup, userOpHash);
validationData = 0; // Validation success and price is valid indefinetly
}
}
Expand All @@ -562,15 +571,15 @@ contract BiconomyTokenPaymaster is
address userOpSender,
address tokenAddress,
uint256 prechargedAmount,
uint192 tokenPrice,
uint256 appliedPriceMarkup,
uint256 tokenPrice,
uint32 appliedPriceMarkup,
bytes32 userOpHash
) = abi.decode(context, (address, address, uint256, uint192, uint256, bytes32));
) = abi.decode(context, (address, address, uint256, uint256, uint32, bytes32));

// Calculate the actual cost in tokens based on the actual gas cost and the token price
uint256 actualTokenAmount = (
(actualGasCost + (unaccountedGas * actualUserOpFeePerGas)) * appliedPriceMarkup * tokenPrice
) / (1e18 * _PRICE_DENOMINATOR);
) / (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
if (prechargedAmount > actualTokenAmount) {
// If the user was overcharged, refund the excess tokens
uint256 refundAmount = prechargedAmount - actualTokenAmount;
Expand All @@ -580,7 +589,7 @@ contract BiconomyTokenPaymaster is

// Todo: Review events and what we need to emit.
emit PaidGasInTokens(
userOpSender, tokenAddress, actualGasCost, actualTokenAmount, appliedPriceMarkup, userOpHash
userOpSender, tokenAddress, actualGasCost, actualTokenAmount, appliedPriceMarkup, tokenPrice, userOpHash
);
}

Expand All @@ -596,8 +605,8 @@ contract BiconomyTokenPaymaster is
}

// Calculate price by using token and native oracle
uint192 tokenPrice = _fetchPrice(tokenInfo.oracle);
uint192 nativeAssetPrice = _fetchPrice(nativeAssetToUsdOracle);
uint256 tokenPrice = _fetchPrice(tokenInfo.oracle);
uint256 nativeAssetPrice = _fetchPrice(nativeAssetToUsdOracle);

// Adjust to token decimals
price = (nativeAssetPrice * tokenInfo.decimals) / tokenPrice;
Expand All @@ -608,15 +617,15 @@ contract BiconomyTokenPaymaster is
/// @param oracle The oracle contract to fetch the price from.
/// @return price The latest price fetched from the oracle.
/// Note: We could do this using oracle aggregator, so we can also use Pyth. or Twap based oracle and just not chainlink.
function _fetchPrice(IOracle oracle) internal view returns (uint192 price) {
function _fetchPrice(IOracle oracle) internal view returns (uint256 price) {
(, int256 answer,, uint256 updatedAt,) = oracle.latestRoundData();
if (answer <= 0) {
revert OraclePriceNotPositive();
}
if (updatedAt < block.timestamp - priceExpiryDuration) {
revert OraclePriceExpired();
}
price = uint192(int192(answer));
price = uint256(answer);
}

function _withdrawERC20(IERC20 token, address target, uint256 amount) private {
Expand Down
2 changes: 0 additions & 2 deletions contracts/token/swaps/Uniswapper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import "@uniswap/v3-periphery/contracts/interfaces/IPeripheryPayments.sol";
* @notice Based on Infinitism's Uniswap Helper contract
*/
abstract contract Uniswapper {
uint256 private constant _SWAP_PRICE_DENOMINATOR = 1e26;

/// @notice The Uniswap V3 SwapRouter contract
ISwapRouter public immutable uniswapRouter;

Expand Down
2 changes: 1 addition & 1 deletion test/base/TestBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
uint48 validUntil;
uint48 validAfter;
address tokenAddress;
uint128 tokenPrice;
uint256 tokenPrice;
uint32 externalPriceMarkup;
}

Expand Down
Loading

0 comments on commit 04580de

Please sign in to comment.