Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Layer2 AA Migration #406

Merged
merged 17 commits into from
Oct 24, 2023
Merged

Layer2 AA Migration #406

merged 17 commits into from
Oct 24, 2023

Conversation

zhoujia6139
Copy link
Contributor

Security Checklist

  • 1. Re-Entrancy
  • 2. Arithmetic Over/Under Flows
  • 3. Unexpected Ether
  • 4. Delegatecall
  • 5. Default Visibilities
  • 6. Entropy Illusion
  • 7. External Contract Referencing
  • 8. Short Address/Parameter Attack (off chain)
  • 9. Unchecked CALL Return Values
  • 10. Race Conditions / Front Running
  • 11. Denial Of Service (DOS)
  • 12. Block Timestamp Manipulation
  • 13. Constructors with Care
  • 14. Uninitialized Storage Pointers
  • 15. Floating Points and Precision
  • 16. Tx.Origin Authentication
  • 17. Address.isContract Re-Entrancy via Constructor

⚠️ NOTES ⚠️

Make sure to think about each of these exploits in this PR.

@zhoujia6139 zhoujia6139 requested a review from a team as a code owner August 25, 2023 08:40
.cache();
currentReserve.updateState(reserveCache);
vars.xTokenAddresses[j] = reserveCache.xTokenAddress;
vars.reserveConfigurations[j] = reserveCache

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest change vars.reserveConfigurations to vars.reserveAssetType, to avoid call getAssetType() each time.

* @notice Defines the basic interface for an ParaSpace Pool.
**/
interface IPoolAAPositionMover {
function positionMoveToAA(uint256 salt) external returns (address);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we define also events here so that typescript can generate correct typings

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.0;

import {ParaVersionedInitializable} from "../libraries/paraspace-upgradeability/ParaVersionedInitializable.sol";
Copy link
Contributor

@GopherJ GopherJ Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cannot we reuse PoolPositionMover?

bool isCollateral = nToken.isUsedAsCollateral(tokenId);
nToken.transferOnLiquidation(user, aaAccount, tokenId);
if (isCollateral) {
nToken.setIsUsedAsCollateral(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we move this out of the loop and use batchSetIsUsedAsCollateral?

@@ -119,6 +122,31 @@ export const step_06 = async (verify = false) => {
);
}

const accountFactory =
(await getContractAddressInDb(eContractid.AccountFactory)) ||
(await deployAccountFactory(zeroAddress()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we assume that AccountFactory is already deployed then we can use getAccountFactory instead


DataTypes.ReserveCache memory reserveCache = currentReserve
.cache();
currentReserve.updateState(reserveCache);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateState is not necessary , since rate is not changed.

IPToken pToken = IPToken(vars.xTokenAddresses[j]);
uint256 balance = pToken.balanceOf(user);
if (balance > 0) {
DataTypes.TimeLockParams memory timeLockParams;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should move timeLockParams out of loop

uint256 balance = pToken.balanceOf(user);
if (balance > 0) {
DataTypes.TimeLockParams memory timeLockParams;
pToken.burn(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sAPE will revert

for (uint256 k = 0; k < balance; k++) {
uint256 tokenId = nToken.tokenOfOwnerByIndex(user, k);
bool isCollateral = nToken.isUsedAsCollateral(tokenId);
nToken.transferOnLiquidation(user, aaAccount, tokenId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if user have staking NFT in current account , user may use positionMoveToAA to keep all staking amount of $Ape in current account , move others token to AA.


DataTypes.ReserveCache memory reserveCache = currentReserve
.cache();
_calculateLiquidityAndDebtIndex(reserveCache);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if assetType is NFT should skip the _calculateLiquidityAndDebtIndex(reserveCache)

}

if (vars.xTokenAddresses[j] == address(0)) {
DataTypes.ReserveData storage currentReserve = ps._reserves[

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest move "address currentReserveAddress = ps._reservesList[j];"to here

@GopherJ GopherJ self-requested a review October 24, 2023 07:49
GopherJ
GopherJ previously approved these changes Oct 24, 2023
@GopherJ GopherJ merged commit eb89afb into main Oct 24, 2023
2 of 4 checks passed
@GopherJ GopherJ deleted the migration_to_aa branch October 24, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants