Conversation
added crosschain vaults and escrows
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| if(currBalance<amountRequested){ | ||
| uint256 amountToLiquidate = amountRequested - currBalance; | ||
| totalStrategyHoldings -= amountToLiquidate; | ||
| parentVault.withdraw(amountToLiquidate, address(this), address(this)); |
There was a problem hiding this comment.
This should withdraw in proportion to tvl. if the parent vault has a loss in the system. then it shouldn't withdraw requested amount. should withdraw in ration of tvl and amount requested. otherwise person withdrawing last will pay the whole gas fees.
| } | ||
|
|
||
| // function to withdraw tokens from the contract | ||
| function withdrawToken(address _token, uint256 _amount) external onlyGovernance{ |
There was a problem hiding this comment.
we use sweep to withdraw token from vault. and should transfer all assets of that token.
There was a problem hiding this comment.
better to keep it in BaseVaultV2.
| } | ||
|
|
||
| function setParentVault(address _parentVault) external onlyGovernance { | ||
| parentVault = Vault(_parentVault); |
There was a problem hiding this comment.
Need to have a check if it has the same assets or not. Also if its swapping the existing vault or not. should have the same gov address.
There was a problem hiding this comment.
in case of swapping existing one. should withdraw all from existing parent vault.
| import {BridgeEscrow} from "../BridgeEscrow.sol"; | ||
| import {L1Vault} from "src/vaults/cross-chain-vault/L1Vault.sol"; | ||
|
|
||
| interface IBaseBridge { |
There was a problem hiding this comment.
please use the interface file in interface directory.
|
|
||
| // TODO: change to mainnet address | ||
| // IBaseBridge public baseBridge = IBaseBridge(payable(0x49048044D57e1C92A77f79988d21Fa8fAF74E97e)); // mainnet | ||
| IBaseBridge public baseBridge = IBaseBridge(payable(0x49f53e41452C74589E85cA1677426Ba426459e85)); // testnet |
There was a problem hiding this comment.
should set on constructor.
There was a problem hiding this comment.
This file is similar as existing L2Wormhole router. Do we need this? or extend existing one?
There was a problem hiding this comment.
same comment as prev one.
| shares = previewDeposit(assets); | ||
| _deposit(_msgSender(), receiver, assets, shares); | ||
| uint256 amountToReturn = MathUpgradeable.min(assets, _asset.balanceOf(address(this))); | ||
| _asset.safeTransfer(receiver, amountToReturn); |
There was a problem hiding this comment.
Please remove this test code from PR.
| } | ||
|
|
||
| // function to withdraw tokens from the contract | ||
| function withdrawToken(address _token, uint256 _amount) external onlyGovernance{ |
There was a problem hiding this comment.
should be sweep in base vault. and transfer whole asset in case on emergency.
There was a problem hiding this comment.
This file is similar as L2Vault. except TransferToL1 method. We can use the existing one with this new transfer method.
Also can put this percent in escrow and trigger this from escrow.
| wormholeRouter = _router; | ||
| } | ||
|
|
||
| function sweep() external onlyGovernance { |
There was a problem hiding this comment.
Transferring full amount is nice. should have a token params in case of transfer any erc20 assets.
There was a problem hiding this comment.
Do we still need this base vault
| } | ||
|
|
||
| function setParentVault(address _parentVault) external onlyGovernance { | ||
| Vault temp = Vault(_parentVault); |
There was a problem hiding this comment.
Should rename this to something like parentVault and params parentVaultAddress
| baseBridge.depositTransaction{value: amountToBridge}( | ||
| l2EscrowAddress, | ||
| amountToBridge, | ||
| 100000, |
There was a problem hiding this comment.
Better to have named variable for this constants.
| IBaseBridge public baseBridge; | ||
| // IBaseBridge public baseBridge = IBaseBridge(payable(0x49f53e41452C74589E85cA1677426Ba426459e85)); // testnet | ||
| address public l2EscrowAddress; | ||
| IWETH public constant WETH = IWETH(payable(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2)); //mainnet |
There was a problem hiding this comment.
This should set as vault assets.
| acrossBridge = IAcrossBridge(payable(0x09aea4b2242abC8bb4BB78D537A67a245A7bEC64)); | ||
| } | ||
|
|
||
| fallback() external payable {} |
There was a problem hiding this comment.
Do we need fallback and receive both?
There was a problem hiding this comment.
IDE/ compiler gives warning if i only have one
| constructor(L2Vault _vault) BridgeEscrow(_vault) { | ||
| vault = _vault; | ||
| asset.safeApprove(address(acrossBridge), type(uint256).max); | ||
| acrossBridge = IAcrossBridge(payable(0x09aea4b2242abC8bb4BB78D537A67a245A7bEC64)); |
There was a problem hiding this comment.
We can pass it as params in constructor.
| ERC20(_token).safeTransfer(governance, _amount); | ||
| } | ||
|
|
||
| function setAcrossBridge(address _acrossBridgeAddress) external onlyGovernance { |
There was a problem hiding this comment.
Should have a check to validate if this is a valid bridge contract.
There was a problem hiding this comment.
Is there even a way to make sure? I can try to see if some of the read variables in the ABI return something but those can still be spoofed with a similar abi
| l1EscrowAddress, | ||
| address(WETH), | ||
| amountToWithdraw, | ||
| 1, |
| ); | ||
|
|
||
| bytes memory delimiterAndReferrer = abi.encodePacked( | ||
| hex"d00dfeeddeadbeef", |
There was a problem hiding this comment.
using direct hex is fine. Please use comment for the original text.
| * @author Affine Devs. Inspired by OpenZeppelin and Rari-Capital. | ||
| */ | ||
|
|
||
| contract L2VaultBase is |
There was a problem hiding this comment.
Can't we just do L2VaultBase is L2Vault here ?
2 participants
No description provided.