Skip to content

Commit

Permalink
Merge pull request #81 from bcnmy/fix/chainlight-remediations
Browse files Browse the repository at this point in the history 
Fix/chainlight remediations
  • Loading branch information
@livingrockrises
livingrockrises authored Feb 14, 2024
2 parents b9e1876 + 5d95cd6 commit e0a87f7
Show file tree
Hide file tree
Showing 20 changed files with 196 additions and 198 deletions.
2 changes: 2 additions & 0 deletions contracts/interfaces/paymasters/IBiconomyTokenPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ interface IBiconomyTokenPaymaster {
*/
event FeeReceiverChanged(address indexed _oldfeeReceiver, address indexed _newfeeReceiver, address indexed _actor);

event TokensWithdrawn(address indexed _token, address indexed _to, uint256 indexed _amount, address actor);

/**
* Designed to enable tracking how much fees were charged from the sender and in which ERC20 token
* More information can be emitted like exchangeRate used, what was the source of exchangeRate etc
Expand Down
10 changes: 5 additions & 5 deletions contracts/libs/MathLib.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@ pragma solidity ^0.8.23;
library MathLib {
function minuint256(uint256 a, uint256 b) internal pure returns (uint256 result) {
assembly {
result := xor(b, mul(xor(b, a), gt(a, b)))
}
result := xor(b, mul(xor(b, a), gt(b, a)))
}
}

function maxuint256(uint256 a, uint256 b) internal pure returns (uint256 result) {
assembly {
result := xor(a, mul(xor(a, b), gt(b, a)))
}
}

function minuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
function minuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
assembly {
result := xor(b, mul(xor(b, a), gt(a, b)))
}
result := xor(b, mul(xor(b, a), gt(b, a)))
}
}

function maxuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
assembly {
Expand Down
179 changes: 106 additions & 73 deletions contracts/token/BiconomyTokenPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@account-abstraction/contracts/core/Helpers.sol" as Helpers;
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import "../utils/SafeTransferLib.sol";
import {MathLib} from "../libs/MathLib.sol";
import {TokenPaymasterErrors} from "./TokenPaymasterErrors.sol";
import "@openzeppelin/contracts/utils/Address.sol";
import {OracleAggregator} from "./oracles/OracleAggregator.sol";
Expand Down Expand Up @@ -79,6 +80,10 @@ contract BiconomyTokenPaymaster is
}
}

receive() external payable {
emit Received(msg.sender, msg.value);
}

/**
* @dev Set a new verifying signer address.
* Can only be called by the owner of the contract.
Expand Down Expand Up @@ -180,11 +185,12 @@ contract BiconomyTokenPaymaster is
onlyOwner
nonReentrant
{
if (token.length != amount.length) {
uint256 tokLen = token.length;
if (tokLen != amount.length) {
revert TokensAndAmountsLengthMismatch();
}
unchecked {
for (uint256 i; i < token.length;) {
for (uint256 i; i < tokLen;) {
_withdrawERC20(token[i], target, amount[i]);
++i;
}
Expand All @@ -198,7 +204,8 @@ contract BiconomyTokenPaymaster is
*/
function withdrawMultipleERC20Full(IERC20[] calldata token, address target) public payable onlyOwner nonReentrant {
unchecked {
for (uint256 i; i < token.length;) {
uint256 tokLen = token.length;
for (uint256 i; i < tokLen;) {
uint256 amount = token[i].balanceOf(address(this));
_withdrawERC20(token[i], target, amount);
++i;
Expand Down Expand Up @@ -285,71 +292,6 @@ contract BiconomyTokenPaymaster is
signature = paymasterAndData[VALID_PND_OFFSET + 53:];
}

function _getRequiredPrefund(UserOperation calldata userOp) internal view returns (uint256 requiredPrefund) {
unchecked {
uint256 requiredGas =
userOp.callGasLimit + userOp.verificationGasLimit + userOp.preVerificationGas + unaccountedEPGasOverhead;

requiredPrefund = requiredGas * userOp.maxFeePerGas;
}
}

/**
* @dev Verify that an external signer signed the paymaster data of a user operation.
* The paymaster data is expected to be the paymaster address, request data and a signature over the entire request parameters.
* paymasterAndData: hexConcat([paymasterAddress, priceSource, abi.encode(validUntil, validAfter, feeToken, exchangeRate, priceMarkup), signature])
* @param userOp The UserOperation struct that represents the current user operation.
* userOpHash The hash of the UserOperation struct.
* @param requiredPreFund The required amount of pre-funding for the paymaster.
* @return context A context string returned by the entry point after successful validation.
* @return validationData An integer returned by the entry point after successful validation.
*/
function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 requiredPreFund)
internal
view
override
returns (bytes memory context, uint256 validationData)
{
(requiredPreFund);

(
ExchangeRateSource priceSource,
uint48 validUntil,
uint48 validAfter,
address feeToken,
uint128 exchangeRate,
uint32 priceMarkup,
bytes calldata signature
) = parsePaymasterAndData(userOp.paymasterAndData);

bytes32 _hash = getHash(userOp, priceSource, validUntil, validAfter, feeToken, exchangeRate, priceMarkup)
.toEthSignedMessageHash();

//don't revert on signature failure: return SIG_VALIDATION_FAILED
if (verifyingSigner != _hash.recover(signature)) {
// empty context and sigFailed true
return (context, Helpers._packValidationData(true, validUntil, validAfter));
}

address account = userOp.getSender();

// This model assumes irrespective of priceSource exchangeRate is always sent from outside
// for below checks you would either need maxCost or some exchangeRate

uint256 btpmRequiredPrefund = _getRequiredPrefund(userOp);

uint256 tokenRequiredPreFund = (btpmRequiredPrefund * exchangeRate) / 10 ** 18;
require(priceMarkup <= 2e6, "BTPM: price markup percentage too high");
require(
IERC20(feeToken).balanceOf(account) >= ((tokenRequiredPreFund * priceMarkup) / PRICE_DENOMINATOR),
"BTPM: account does not have enough token balance"
);

context = abi.encode(account, feeToken, priceSource, exchangeRate, priceMarkup, userOpHash);

return (context, Helpers._packValidationData(false, validUntil, validAfter));
}

/**
* @dev Executes the paymaster's payment conditions
* @param mode tells whether the op succeeded, reverted, or if the op succeeded but cause the postOp to revert
Expand All @@ -362,6 +304,8 @@ contract BiconomyTokenPaymaster is
ExchangeRateSource priceSource;
uint128 exchangeRate;
uint32 priceMarkup;
uint256 maxFeePerGas;
uint256 maxPriorityFeePerGas;
bytes32 userOpHash;
assembly ("memory-safe") {
let offset := context.offset
Expand All @@ -381,6 +325,12 @@ contract BiconomyTokenPaymaster is
priceMarkup := calldataload(offset)
offset := add(offset, 0x20)

maxFeePerGas := calldataload(offset)
offset := add(offset, 0x20)

maxPriorityFeePerGas := calldataload(offset)
offset := add(offset, 0x20)

userOpHash := calldataload(offset)
}

Expand All @@ -391,11 +341,16 @@ contract BiconomyTokenPaymaster is
if (result != 0) effectiveExchangeRate = result;
}

uint256 effectiveGasPrice = getGasPrice(
maxFeePerGas,
maxPriorityFeePerGas
);

// We could either touch the state for BASEFEE and calculate based on maxPriorityFee passed (to be added in context along with maxFeePerGas) or just use tx.gasprice
uint256 charge; // Final amount to be charged from user account
{
uint256 actualTokenCost =
((actualGasCost + (unaccountedEPGasOverhead * tx.gasprice)) * effectiveExchangeRate) / 1e18;
((actualGasCost + (unaccountedEPGasOverhead * effectiveGasPrice)) * effectiveExchangeRate) / 1e18;
charge = ((actualTokenCost * priceMarkup) / PRICE_DENOMINATOR);
}

Expand Down Expand Up @@ -425,12 +380,90 @@ contract BiconomyTokenPaymaster is
}
}

function _getRequiredPrefund(UserOperation calldata userOp) internal view returns (uint256 requiredPrefund) {
unchecked {
uint256 requiredGas =
userOp.callGasLimit + userOp.verificationGasLimit + userOp.preVerificationGas + unaccountedEPGasOverhead;

requiredPrefund = requiredGas * userOp.maxFeePerGas;
}
}

// Note: do not use this in validation phase
function getGasPrice(
uint256 maxFeePerGas,
uint256 maxPriorityFeePerGas
) internal view returns (uint256) {
if (maxFeePerGas == maxPriorityFeePerGas) {
//legacy mode (for networks that don't support basefee opcode)
return maxFeePerGas;
}
return
MathLib.minuint256(
maxFeePerGas,
maxPriorityFeePerGas + block.basefee
);
}

/**
* @dev Verify that an external signer signed the paymaster data of a user operation.
* The paymaster data is expected to be the paymaster address, request data and a signature over the entire request parameters.
* paymasterAndData: hexConcat([paymasterAddress, priceSource, abi.encode(validUntil, validAfter, feeToken, exchangeRate, priceMarkup), signature])
* @param userOp The UserOperation struct that represents the current user operation.
* userOpHash The hash of the UserOperation struct.
* @param requiredPreFund The required amount of pre-funding for the paymaster.
* @return context A context string returned by the entry point after successful validation.
* @return validationData An integer returned by the entry point after successful validation.
*/
function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 requiredPreFund)
internal
view
override
returns (bytes memory context, uint256 validationData)
{
(requiredPreFund);

(
ExchangeRateSource priceSource,
uint48 validUntil,
uint48 validAfter,
address feeToken,
uint128 exchangeRate,
uint32 priceMarkup,
bytes calldata signature
) = parsePaymasterAndData(userOp.paymasterAndData);

bytes32 _hash = getHash(userOp, priceSource, validUntil, validAfter, feeToken, exchangeRate, priceMarkup)
.toEthSignedMessageHash();

//don't revert on signature failure: return SIG_VALIDATION_FAILED
if (verifyingSigner != _hash.recover(signature)) {
// empty context and sigFailed true
return (context, Helpers._packValidationData(true, validUntil, validAfter));
}

address account = userOp.getSender();

// This model assumes irrespective of priceSource exchangeRate is always sent from outside
// for below checks you would either need maxCost or some exchangeRate

uint256 btpmRequiredPrefund = _getRequiredPrefund(userOp);

uint256 tokenRequiredPreFund = (btpmRequiredPrefund * exchangeRate) / 10 ** 18;
require(priceMarkup <= 2e6, "BTPM: price markup percentage too high");
require(
IERC20(feeToken).balanceOf(account) >= ((tokenRequiredPreFund * priceMarkup) / PRICE_DENOMINATOR),
"BTPM: Insufficient token balance"
);

context = abi.encode(account, feeToken, priceSource, exchangeRate, priceMarkup, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, userOpHash);

return (context, Helpers._packValidationData(false, validUntil, validAfter));
}

function _withdrawERC20(IERC20 token, address target, uint256 amount) private {
if (target == address(0)) revert CanNotWithdrawToZeroAddress();
SafeTransferLib.safeTransfer(address(token), target, amount);
}

receive() external payable {
emit Received(msg.sender, msg.value);
emit TokensWithdrawn(address(token), target, amount, msg.sender);
}
}
13 changes: 7 additions & 6 deletions contracts/token/oracles/IOracleAggregator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@
pragma solidity ^0.8.23;

interface IOracleAggregator {
error MismatchInBaseAndQuoteDecimals();
error InvalidPriceFromRound();
error LatestRoundIncomplete();
error PriceFeedStale();
error OracleAddressCannotBeZero();

struct TokenInfo {
uint8 tokenDecimals;
uint24 priceUpdateThreshold;
address tokenOracle;
address nativeOracle;
bool isDerivedFeed;
}

error MismatchInBaseAndQuoteDecimals();
error InvalidPriceFromRound();
error LatestRoundIncomplete();
error PriceFeedStale();
error OracleAddressCannotBeZero();

function getTokenValueOfOneNativeToken(address _token) external view returns (uint128 exchangeRate);
}
26 changes: 15 additions & 11 deletions contracts/token/oracles/OracleAggregator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity ^0.8.23;

import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "./IOracleAggregator.sol";
import "./FeedInterface.sol";

Expand All @@ -14,10 +15,10 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {

function setTokenOracle(
address token,
uint8 tokenDecimals,
address tokenOracle,
address nativeOracle,
bool isDerivedFeed
bool isDerivedFeed,
uint24 priceUpdateThreshold
) external onlyOwner {
if (tokenOracle == address(0)) revert OracleAddressCannotBeZero();
if (nativeOracle == address(0)) revert OracleAddressCannotBeZero();
Expand All @@ -27,8 +28,9 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
if (decimals1 != decimals2) revert MismatchInBaseAndQuoteDecimals();
tokensInfo[token].tokenOracle = tokenOracle;
tokensInfo[token].nativeOracle = nativeOracle;
tokensInfo[token].tokenDecimals = tokenDecimals;
tokensInfo[token].tokenDecimals = ERC20(token).decimals();
tokensInfo[token].isDerivedFeed = isDerivedFeed;
tokensInfo[token].priceUpdateThreshold = priceUpdateThreshold;
}

/**
Expand All @@ -50,18 +52,19 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
view
returns (uint256 tokenPrice, uint8 tokenOracleDecimals, uint8 tokenDecimals, bool isError)
{
TokenInfo storage tokenInfo = tokensInfo[token];
TokenInfo memory tokenInfo = tokensInfo[token];
tokenDecimals = tokenInfo.tokenDecimals;
uint24 priceUpdateThreshold = tokenInfo.priceUpdateThreshold;

if (tokenInfo.isDerivedFeed) {
(uint256 price1, bool isError1) = fetchPrice(FeedInterface(tokenInfo.nativeOracle));
(uint256 price2, bool isError2) = fetchPrice(FeedInterface(tokenInfo.tokenOracle));
(uint256 price1, bool isError1) = fetchPrice(FeedInterface(tokenInfo.nativeOracle), priceUpdateThreshold);
(uint256 price2, bool isError2) = fetchPrice(FeedInterface(tokenInfo.tokenOracle), priceUpdateThreshold);
isError = isError1 || isError2;
if (isError) return (0, 0, 0, isError);
tokenPrice = (price2 * (10 ** 18)) / price1;
tokenOracleDecimals = 18;
} else {
(tokenPrice, isError) = fetchPrice(FeedInterface(tokenInfo.tokenOracle));
(tokenPrice, isError) = fetchPrice(FeedInterface(tokenInfo.tokenOracle), priceUpdateThreshold);
tokenOracleDecimals = FeedInterface(tokenInfo.tokenOracle).decimals();
}
}
Expand All @@ -70,17 +73,18 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
* @dev This function is used to get the latest price from the tokenOracle or nativeOracle.
* @notice Fetches the latest price from the given Oracle.
* @param _oracle The Oracle contract to fetch the price from.
* @param _priceUpdateThreshold The time after which the price is considered stale.
* @return price The latest price fetched from the Oracle.
*/
function fetchPrice(FeedInterface _oracle) internal view returns (uint256 price, bool isError) {
function fetchPrice(FeedInterface _oracle, uint24 _priceUpdateThreshold) internal view returns (uint256 price, bool isError) {
try _oracle.latestRoundData() returns (
uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound
) {
// validateRound
if (answer <= 0) return (0, true);
// 2 days old price is considered stale since the price is updated every 24 hours
if (updatedAt < block.timestamp - 60 * 60 * 24 * 2) return (0, true);
if (answeredInRound < roundId) return (0, true);
// price older than set _priceUpdateThreshold is considered stale
// _priceUpdateThreshold for oracle feed is usually heartbeat interval + block time + buffer
if (updatedAt < block.timestamp - _priceUpdateThreshold) return (0, true);
price = uint256(answer);
return (price, false);
} catch Error(string memory reason) {
Expand Down
Loading

0 comments on commit e0a87f7

Please sign in to comment.