Skip to content

Commit

Permalink
Implementing AA audit feedback (#1023)
Browse files Browse the repository at this point in the history 
* fixed AA: using oracle method that could return 0

* combine

* tests for AA for latestRound

* final-changes

* fix-integration-tests

* Update packages/boba/account-abstraction/contracts/samples/BobaDepositPaymaster.sol

Co-authored-by: Souradeep Das <[email protected]>

* oz downgrade

* undo-solc-change

* Update packages/boba/account-abstraction/contracts/bundler/EntryPointWrapper.sol

Co-authored-by: Souradeep Das <[email protected]>

* Update packages/boba/account-abstraction/contracts/bundler/EntryPointWrapper.sol

Co-authored-by: Souradeep Das <[email protected]>

---------

Co-authored-by: Souradeep Das <[email protected]>
  • Loading branch information
@wsdt @souradeep-das
wsdt and souradeep-das authored Jul 14, 2023
1 parent dc743e1 commit 2804473
Show file tree
Hide file tree
Showing 9 changed files with 136 additions and 70 deletions.
15 changes: 12 additions & 3 deletions packages/boba/account-abstraction/contracts/bundler/EntryPointWrapper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,12 @@ contract EntryPointWrapper {
}
}

// helper function to slice function signature from return data
/** @dev Helper function to slice function signature from return data
* @param _bytes: returnData
* @param _start: where to start the slice
* @param _length: Length of slice
* @return Sliced returnData
*/
function slice(
bytes memory _bytes,
uint256 _start,
Expand Down Expand Up @@ -232,7 +237,9 @@ contract EntryPointWrapper {
return tempBytes;
}

// helper function to get multiple userOpHashes in a single call, used by the bundler
/** @dev Helper function to get multiple userOpHashes in a single call, used by the bundler
* @param entryPoint: EntryPoint interface/address
* @param userOps: User operations to return user op hash for. */
function getUserOpHashes(IEntryPoint entryPoint, UserOperation[] memory userOps) public view returns (bytes32[] memory ret) {
ret = new bytes32[](userOps.length);
for (uint i = 0; i < userOps.length; i++) {
Expand All @@ -241,7 +248,9 @@ contract EntryPointWrapper {
return ret;
}

// helper function to get hashed accounthash of addresses, used by the bundler
/** @dev Helper function to get hashed accounthash of addresses, used by the bundler
* @param addresses: Addresses to return code hashes for.
* @return Hash of code hashes */
function getCodeHashes(address[] memory addresses) public view returns (bytes32) {
bytes32[] memory hashes = new bytes32[](addresses.length);
for (uint i = 0; i < addresses.length; i++) {
Expand Down
21 changes: 17 additions & 4 deletions packages/boba/account-abstraction/contracts/samples/BobaDepositPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,11 @@ contract BobaDepositPaymaster is BasePaymaster {
uint8 tokenDecimals;
}

/** @dev Max age of token/eth price returned by latestRoundData() */
uint256 public constant MAX_AGE_ASSET_PRICE = 10800; // default 3h TBD

//calculated cost of the postOp
uint256 constant public COST_OF_POST = 35000;
uint256 public constant COST_OF_POST = 35000;
address public constant QUOTE_USD = 0x0000000000000000000000000000000000000348;

// for alt-l1, treat this as the native token
Expand Down Expand Up @@ -140,12 +143,22 @@ contract BobaDepositPaymaster is BasePaymaster {
Oracle memory oracleInfo = oracles[token];
require(oracleInfo.feedRegistry != NULL_ORACLE, "DepositPaymaster: unsupported token");
address base = oracleInfo.tokenBase;
uint256 ethPrice = uint256(oracles[L2_ETH].feedRegistry.latestAnswer(oracles[L2_ETH].tokenBase, QUOTE_USD));
uint256 tokenPrice = uint256(oracleInfo.feedRegistry.latestAnswer(base, QUOTE_USD));

uint256 currTimestamp = block.timestamp;
(uint80 roundId, int256 price,,uint256 priceUpdatedAt,) = oracles[L2_ETH].feedRegistry.latestRoundData(oracles[L2_ETH].tokenBase, QUOTE_USD);
require(roundId != 0 && price > 0 && priceUpdatedAt != 0 && priceUpdatedAt <= currTimestamp, "ETH round failed");
require((currTimestamp - priceUpdatedAt) < MAX_AGE_ASSET_PRICE, "ETH price expired");
uint256 ethPrice = uint256(price);

(roundId, price,, priceUpdatedAt,) = oracleInfo.feedRegistry.latestRoundData(base, QUOTE_USD);
require(roundId != 0 && price > 0 && priceUpdatedAt != 0 && priceUpdatedAt <= currTimestamp, "Token round failed");
require((currTimestamp - priceUpdatedAt) < MAX_AGE_ASSET_PRICE, "Token price expired");
uint256 tokenPrice = uint256(price);

uint256 ethPriceDecimals = uint256(oracles[L2_ETH].feedRegistry.decimals(oracles[L2_ETH].tokenBase, QUOTE_USD));
uint256 tokenPriceDecimals = uint256(oracleInfo.feedRegistry.decimals(base, QUOTE_USD));
uint256 requiredAmount = (ethBought * ethPrice * (10**tokenPriceDecimals)) / (tokenPrice * (10**ethPriceDecimals));
// there is no requiredAmount = 0 check, priceRatio from oracle shouldnt exceed ethBought
require(requiredAmount != 0, "Required amount zero"); // Since decimal oracle feeds could return different decimals that ruins assumption requiredAmount cannot be 0
return ((requiredAmount * (10**oracleInfo.tokenDecimals)) / (10**oracles[L2_ETH].tokenDecimals));
}

Expand Down
110 changes: 64 additions & 46 deletions packages/boba/account-abstraction/contracts/test/mocks/MockFeedRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,92 +6,110 @@ import "../../samples/IBobaStraw.sol";

contract MockFeedRegistry is IBobaStraw {

int256 public fixedReturnValue = 45;
uint8 public decimalsOverride = 8;
int256 public fixedReturnValue = 45;
uint8 public decimalsOverride = 8;
uint80 public fixedRoundId = 1;
uint256 public fixedUpdatedAt = block.timestamp;

function latestRoundData(
address base,
address quote
)
function latestRoundData(
address base,
address quote
)
external
view
returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound // deprecated
) {
return (0,0,0,0,0);
return (fixedRoundId, fixedReturnValue, 0, fixedUpdatedAt, 0);
}

function getRoundData(
address base,
address quote,
uint80 _roundId
)
function getRoundData(
address base,
address quote,
uint80 _roundId
)
external
view
returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
) {
return (0,0,0,0,0);
return (0, 0, 0, 0, 0);
}

// V2 AggregatorInterface
// V2 AggregatorInterface

function latestAnswer(
address base,
address quote
)
function latestAnswer(
address base,
address quote
)
external
view
returns (
int256 answer
int256 answer
) {
return fixedReturnValue;
}

function latestTimestamp(
address base,
address quote
)
function latestTimestamp(
address base,
address quote
)
external
view
returns (
uint256 timestamp
uint256 timestamp
) {
return 0;
return 0;
}

function decimals(
address base,
address quote
)
function decimals(
address base,
address quote
)
external
view
returns (
uint8
uint8
) {
return decimalsOverride;
return decimalsOverride;
}

function updateFixedRetunValue(
int256 newValue
function updateFixedReturnValue(
int256 newValue
)
external
external
{
fixedReturnValue = newValue;
}

function updateFixedRoundId(
uint80 newValue
)
external
{
fixedRoundId = newValue;
}

function updateFixedUpdatedAt(
uint256 newValue
)
external
{
fixedUpdatedAt = newValue;
}

function updateDecimals(
uint8 newDecimals
uint8 newDecimals
)
external
external
{
decimalsOverride = newDecimals;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/boba/account-abstraction/contracts/test/mocks/MockGasPriceOracle.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ contract MockGasPriceOracle is IBobaGasPriceOracle {
return fixedReturnValue;
}

function updateFixedRetunValue(uint256 newValue) external {
function updateFixedReturnValue(uint256 newValue) external {
fixedReturnValue = newValue;
}

function updateDecimals(uint256 newDecimals) external {
decimalsOverride = newDecimals;
}
}
}
2 changes: 1 addition & 1 deletion ...ount-abstraction/deployments/boba_goerli/solcInputs/dc1c9c8b476960fded4294d8921901a6.json
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/boba/account-abstraction/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
"dependencies": {
"@gnosis.pm/safe-contracts": "^1.3.0",
"@nomiclabs/hardhat-etherscan": "^3.1.0",
"@openzeppelin/contracts": "^4.2.0",
"@openzeppelin/contracts": "^4.8.0",
"@thehubbleproject/bls": "^0.5.1",
"@typechain/hardhat": "^6.1.2",
"@types/mocha": "^9.0.0",
Expand Down
42 changes: 34 additions & 8 deletions packages/boba/account-abstraction/test/deposit-paymaster-boba.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
} from './testutils'
import { fillAndSign } from './UserOp'
import { hexConcat, hexZeroPad, parseEther } from 'ethers/lib/utils'
import {BigNumber} from "ethers";

describe('BobaDepositPaymaster', () => {
let entryPoint: EntryPoint
Expand Down Expand Up @@ -242,12 +243,37 @@ describe('BobaDepositPaymaster', () => {
// oracle returns 1:1 conversion
expect(requiredTokens).to.be.eq(ethBoughtAmount)
})

it('should fail for invalid roundId', async () => {
const prevRoundId = await testEthOracle.fixedRoundId()
await testEthOracle.updateFixedRoundId(0)

const ethBoughtAmount = ethers.utils.parseEther('1')
await expect(bobaDepositPaymaster.getTokenValueOfEthTest(token.address, ethBoughtAmount)).to.be.revertedWith("ETH round failed")

await testEthOracle.updateFixedRoundId(prevRoundId)
})

it('should fail when last update from oracle was too long ago', async () => {
// see constant in BobaDepositPaymaster for current expiration value
const maxAgeUpdatedAt = await bobaDepositPaymaster.MAX_AGE_ASSET_PRICE()
const currBlockTimeStamp = BigNumber.from((await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp);

const prevUpdatedAt = await testEthOracle.fixedUpdatedAt()
await testEthOracle.updateFixedUpdatedAt(currBlockTimeStamp.sub(maxAgeUpdatedAt))

const ethBoughtAmount = ethers.utils.parseEther('1')
await expect(bobaDepositPaymaster.getTokenValueOfEthTest(token.address, ethBoughtAmount)).to.be.revertedWith("ETH price expired")

await testEthOracle.updateFixedUpdatedAt(prevUpdatedAt)
})

it('should return correct conversion on different values', async () => {
// set eth price
await testEthOracle.updateFixedRetunValue(125475500000)
await testEthOracle.updateFixedReturnValue(125475500000)
// set token price
// example boba
await testTokenOracle.updateFixedRetunValue(21612500)
await testTokenOracle.updateFixedReturnValue(21612500)

let ethBoughtAmount = ethers.utils.parseEther('1')
let requiredTokens = await bobaDepositPaymaster.getTokenValueOfEthTest(token.address, ethBoughtAmount)
Expand All @@ -265,10 +291,10 @@ describe('BobaDepositPaymaster', () => {

await bobaDepositPaymaster.addToken(tokenAlt.address, testTokenOracle.address, tokenAlt.address, 6)
// set eth price
await testEthOracle.updateFixedRetunValue(125475500000)
await testEthOracle.updateFixedReturnValue(125475500000)
// set token price
// example usdc, adjust decimals
await testTokenOracle.updateFixedRetunValue(100000000)
await testTokenOracle.updateFixedReturnValue(100000000)

let ethBoughtAmount = ethers.utils.parseEther('1')
let requiredTokens = await bobaDepositPaymaster.getTokenValueOfEthTest(tokenAlt.address, ethBoughtAmount)
Expand All @@ -282,12 +308,12 @@ describe('BobaDepositPaymaster', () => {
})
it('should return correct conversion on different price oracle decimals', async () => {
// set eth price
await testEthOracle.updateFixedRetunValue(125475500000)
await testEthOracle.updateFixedReturnValue(125475500000)
// set token price
// example boba
await testTokenOracle.updateDecimals(6)
// decimal 6
await testTokenOracle.updateFixedRetunValue(216125)
await testTokenOracle.updateFixedReturnValue(216125)

let ethBoughtAmount = ethers.utils.parseEther('1')
let requiredTokens = await bobaDepositPaymaster.getTokenValueOfEthTest(token.address, ethBoughtAmount)
Expand All @@ -307,10 +333,10 @@ describe('BobaDepositPaymaster', () => {
await testTokenOracle.updateDecimals(6)
await bobaDepositPaymaster.addToken(tokenAlt.address, testTokenOracle.address, tokenAlt.address, 6)
// set eth price
await testEthOracle.updateFixedRetunValue(125475500000)
await testEthOracle.updateFixedReturnValue(125475500000)
// set token price
// example usdc, adjust priceOracle decimals to 6
await testTokenOracle.updateFixedRetunValue(1000000)
await testTokenOracle.updateFixedReturnValue(1000000)

let ethBoughtAmount = ethers.utils.parseEther('1')
let requiredTokens = await bobaDepositPaymaster.getTokenValueOfEthTest(tokenAlt.address, ethBoughtAmount)
Expand Down
6 changes: 3 additions & 3 deletions packages/boba/account-abstraction/test/deposit-paymaster-gpo.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,14 @@ describe('GPODepositPaymaster', () => {
gpoDepositPaymaster = await new TestTokenValueGPODepositPaymaster__factory(ethersSigner).deploy(entryPoint.address, token.address, await token.decimals(), gasPriceOracle.address)
})
it('should return correct conversion', async () => {
await gasPriceOracle.updateFixedRetunValue(78125)
await gasPriceOracle.updateFixedReturnValue(78125)
await gasPriceOracle.updateDecimals(1)
const ethBoughtAmount = ethers.utils.parseEther('1')
const requiredTokens = await gpoDepositPaymaster.getTokenValueOfEthTest(ethBoughtAmount)
expect(requiredTokens).to.be.eq('7812500000000000000000')
})
it('should return correct conversion on different priceRatio decimals', async () => {
await gasPriceOracle.updateFixedRetunValue(7812500000)
await gasPriceOracle.updateFixedReturnValue(7812500000)
await gasPriceOracle.updateDecimals(6)

let ethBoughtAmount = ethers.utils.parseEther('1')
Expand All @@ -212,7 +212,7 @@ describe('GPODepositPaymaster', () => {

gpoDepositPaymaster = await new TestTokenValueGPODepositPaymaster__factory(ethersSigner).deploy(entryPoint.address, tokenAlt.address, await tokenAlt.decimals(), gasPriceOracle.address)

await gasPriceOracle.updateFixedRetunValue(78125)
await gasPriceOracle.updateFixedReturnValue(78125)
await gasPriceOracle.updateDecimals(1)

let ethBoughtAmount = ethers.utils.parseEther('1')
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5551,7 +5551,7 @@
resolved "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.3.2.tgz"
integrity sha512-AybF1cesONZStg5kWf6ao9OlqTZuPqddvprc0ky7lrUVOjXeKpmQ2Y9FK+6ygxasb+4aic4O5pneFBfwVsRRRg==

"@openzeppelin/contracts@^4.2.0", "@openzeppelin/contracts@^4.4.0":
"@openzeppelin/contracts@^4.4.0":
version "4.8.0"
resolved "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.8.0.tgz"
integrity sha512-AGuwhRRL+NaKx73WKRNzeCxOCOCxpaqF+kp8TJ89QzAipSwZy/NoflkWaL9bywXFRhIzXt8j38sfF7KBKCPWLw==
Expand All @@ -5561,7 +5561,7 @@
resolved "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.8.1.tgz"
integrity sha512-xQ6eUZl+RDyb/FiZe1h+U7qr/f4p/SrTSQcTPH2bjur3C5DbuW/zFgCU/b1P/xcIaEqJep+9ju4xDRi3rmChdQ==

"@openzeppelin/contracts@^4.9.2":
"@openzeppelin/contracts@^4.8.0", "@openzeppelin/contracts@^4.9.2":
version "4.9.2"
resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.9.2.tgz#1cb2d5e4d3360141a17dbc45094a8cad6aac16c1"
integrity sha512-mO+y6JaqXjWeMh9glYVzVu8HYPGknAAnWyxTRhGeckOruyXQMNnlcW6w/Dx9ftLeIQk6N+ZJFuVmTwF7lEIFrg==
Expand Down

0 comments on commit 2804473

Please sign in to comment.