Instead of calling IVaultRegistry(registry).totalSupply(_vault), which would load again token and id from storage, we could use the token and id we've already loaded.
+import {IFERC1155} from "../interfaces/IFERC1155.sol";
+
/// @title Buyout
/// @author Fractional Art
/// @notice Module contract for vaults to hold buyout pools
@@ -68,7 +70,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
if (current != required) revert InvalidState(required, current);
// Gets total supply of fractional tokens for the vault
- uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+ uint256 totalSupply = IFERC1155(token).totalSupply(id);
// Gets total balance of fractional tokens owned by caller
uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);
@@ -118,6 +120,8 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
(uint256 startTime, , State current, uint256 fractionPrice, , ) = this
.buyoutInfo(_vault);
// Reverts if auction state is not live
+ // @audit make state constant to save gas
+ // this is not going to save much because the var is stored in memory
State required = State.LIVE;
if (current != required) revert InvalidState(required, current);
// Reverts if current time is greater than end time of proposal period
@@ -205,9 +209,10 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
uint256 tokenBalance = IERC1155(token).balanceOf(address(this), id);
// Checks totalSupply of auction pool to determine if buyout is successful or not
+ // @audit would be cheaper to do this than have vault registery load it again from storage
if (
(tokenBalance * 1000) /
- IVaultRegistry(registry).totalSupply(_vault) >
+ IFERC1155(token).totalSupply(id) >
500
) {
// Initializes vault transaction
@@ -264,7 +269,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
IVault(payable(_vault)).execute(supply, data, _burnProof);
// Transfers buyout share amount to caller based on total supply
- uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+ uint256 totalSupply = IFERC1155(token).totalSupply(id);
uint256 buyoutShare = (tokenBalance * ethBalance) /
(totalSupply + tokenBalance);
_sendEthOrWeth(msg.sender, buyoutShare);
@@ -277,7 +282,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
/// @param _burnProof Merkle proof for burning fractional tokens
function redeem(address _vault, bytes32[] calldata _burnProof) external {
// Reverts if address is not a registered vault
- (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
+ (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
if (id == 0) revert NotVault(_vault);
// Reverts if auction state is not inactive
(, , State current, , , ) = this.buyoutInfo(_vault);
@@ -285,7 +290,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
if (current != required) revert InvalidState(required, current);
// Initializes vault transaction
- uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+ uint256 totalSupply = IFERC1155(token).totalSupply(id);
bytes memory data = abi.encodeCall(
ISupply.burn,
(msg.sender, totalSupply)
diff --git a/src/modules/Migration.sol b/src/modules/Migration.sol
index d81d0ef..7d4301f 100644
--- a/src/modules/Migration.sol
+++ b/src/modules/Migration.sol
@@ -78,7 +78,7 @@ contract Migration is
uint256 _targetPrice
) external {
// Reverts if address is not a registered vault
- (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
+ (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
if (id == 0) revert NotVault(_vault);
// Reverts if buyout state is not inactive
(, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
@@ -92,9 +92,7 @@ contract Migration is
proposal.modules = _modules;
proposal.plugins = _plugins;
proposal.selectors = _selectors;
- proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
- _vault
- );
+ proposal.oldFractionSupply = IFERC1155(token).totalSupply(id);
proposal.newFractionSupply = _newFractionSupply;
}
@@ -197,7 +195,7 @@ contract Migration is
// Calculates current price of the proposal based on total supply
uint256 currentPrice = _calculateTotal(
100,
- IVaultRegistry(registry).totalSupply(_vault),
+ IFERC1155(token).totalSupply(id),
proposal.totalEth,
proposal.totalFractions
);
@@ -467,7 +465,7 @@ contract Migration is
(address token, uint256 newFractionId) = IVaultRegistry(registry)
.vaultToToken(newVault);
// Calculates share amount of fractions for the new vault based on the new total supply
- uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
+ uint256 newTotalSupply = IFERC1155(token).totalSupply(newFractionId);
uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
totalInEth;
This would save loading the variable from storage each time.
diff --git a/src/VaultFactory.sol b/src/VaultFactory.sol
index 0902ebb..b08e56e 100644
--- a/src/VaultFactory.sol
+++ b/src/VaultFactory.sol
@@ -12,7 +12,8 @@ contract VaultFactory is IVaultFactory {
/// @dev Use clones library for address types
using Create2ClonesWithImmutableArgs for address;
/// @notice Address of Vault proxy contract
- address public implementation;
+ // @audit can be set to immutable since it doesn't change
+ address immutable public implementation;
/// @dev Internal mapping to track the next seed to be used by an EOA
mapping(address => bytes32) internal nextSeeds;
diff --git a/src/modules/Migration.sol b/src/modules/Migration.sol
index d81d0ef..810fa1d 100644
--- a/src/modules/Migration.sol
+++ b/src/modules/Migration.sol
@@ -34,9 +34,9 @@ contract Migration is
ReentrancyGuard
{
/// @notice Address of Buyout module contract
- address payable public buyout;
+ address payable immutable public buyout;
/// @notice Address of VaultRegistry contract
- address public registry;
+ address immutable public registry;
/// @notice Counter used to assign IDs to new proposals
uint256 public nextId;
/// @notice The length for the migration proposal period
diff --git a/src/modules/protoforms/BaseVault.sol b/src/modules/protoforms/BaseVault.sol
index e02abf0..a8e4476 100644
--- a/src/modules/protoforms/BaseVault.sol
+++ b/src/modules/protoforms/BaseVault.sol
@@ -16,7 +16,7 @@ import {Multicall} from "../../utils/Multicall.sol";
/// @notice Protoform contract for vault deployments with a fixed supply and buyout mechanism
contract BaseVault is IBaseVault, MerkleBase, Minter, Multicall {
/// @notice Address of VaultRegistry contract
- address public registry;
+ address immutable public registry;
/// @notice Initializes registry and supply contracts
/// @param _registry Address of the VaultRegistry contract
Add an initFor function to Vault to save gas in VaultFactory
Instead of doing an init and then transferring ownership (transferOwner takes an extra sstore op), just add to Vault functionality that let's init for another address and use it in VaultFactory.
diff --git a/src/Vault.sol b/src/Vault.sol
index 60c9bff..23e1c66 100644
--- a/src/Vault.sol
+++ b/src/Vault.sol
@@ -22,10 +22,14 @@ contract Vault is IVault, NFTReceiver {
/// @dev Initializes nonce and proxy owner
function init() external {
+ initFor(msg.sender);
+ }
+
+ function initFor(address _owner) public {
if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
nonce = 1;
- owner = msg.sender;
- emit TransferOwnership(address(0), msg.sender);
+ owner = _owner;
+ emit TransferOwnership(address(0), _owner);
}
/// @dev Callback for receiving Ether when the calldata is empty
diff --git a/src/VaultFactory.sol b/src/VaultFactory.sol
index 0902ebb..169113f 100644
--- a/src/VaultFactory.sol
+++ b/src/VaultFactory.sol
@@ -67,10 +67,10 @@ contract VaultFactory is IVaultFactory {
bytes memory data = abi.encodePacked();
vault = implementation.clone(salt, data);
- Vault(vault).init();
+ Vault(vault).initFor(_owner);
// Transfer the ownership from this factory contract to the specified owner.
- Vault(vault).transferOwnership(_owner);
+ // Vault(vault).transferOwnership(_owner);
// Increment the seed.
unchecked {
In the function Buyout.startdepositAmount might be zero (in case that the proposer doesn't hold any fractions)
In the function Buyout.endtokenBalance might be zero (if all fractions were bought)
Since IERC1155.safeTransferFrom costs about 21K gas units on avg (according to the gas report), it's worth checking before that the amount isn't zero.
diff --git a/src/modules/Buyout.sol b/src/modules/Buyout.sol
index 1557233..5420710 100644
--- a/src/modules/Buyout.sol
+++ b/src/modules/Buyout.sol
@@ -72,14 +72,16 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
// Gets total balance of fractional tokens owned by caller
uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);
- // Transfers fractional tokens into the buyout pool
- IERC1155(token).safeTransferFrom(
- msg.sender,
- address(this),
- id,
- depositAmount,
- ""
- );
+ if(depositAmount != 0){
+ // Transfers fractional tokens into the buyout pool
+ IERC1155(token).safeTransferFrom(
+ msg.sender,
+ address(this),
+ id,
+ depositAmount,
+ ""
+ );
+ }
// Calculates price of buyout and fractions
// @dev Reverts with division error if called with total supply of tokens
@@ -225,14 +227,18 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
// Deletes auction info
delete buyoutInfo[_vault];
// Transfers fractions and ether back to proposer of the buyout pool
- IERC1155(token).safeTransferFrom(
- address(this),
- proposer,
- id,
- tokenBalance,
- ""
- );
- _sendEthOrWeth(proposer, ethBalance);
+ if(tokenBalance != 0){
+ IERC1155(token).safeTransferFrom(
+ address(this),
+ proposer,
+ id,
+ tokenBalance,
+ ""
+ );
+ }
+ if(ethBalance != 0){
+ _sendEthOrWeth(proposer, ethBalance);
+ }
// Emits event for ending unsuccessful auction
emit End(_vault, State.INACTIVE, proposer);
}
This one might need further checking and consideration, but caching merkle proof might be cheaper in the long run.
Here's the math:
Verifying merkle proof costs about 0.9K per layer, which would be 2.7K per 3 layers (leafs > 4, current situation in testing), 3.6K for 4 layers (leafs > 8) and 4.5K for 5 layers (leafs > 16)
Loading a cold key from storage should cost about 2.1K, that could be much cheaper (even for 3 layers) in many cases, all we'll need is to cache that leaf once (~21K)
Of course not all leafs would necessarily cached, so in order to not waste gas if it's not - the caller would have to specify if to check cache or not
Here's the suggested code change:
diff --git a/src/Vault.sol b/src/Vault.sol
index 60c9bff..86aa720 100644
--- a/src/Vault.sol
+++ b/src/Vault.sol
@@ -17,6 +17,12 @@ contract Vault is IVault, NFTReceiver {
uint256 public nonce;
/// @dev Minimum reserve of gas units
uint256 private constant MIN_GAS_RESERVE = 5_000;
+
+ error CacheFailed(address source, bytes32 leaf, bytes32 cachedRoot);
+ event Cache(address source, bytes32 leaf, bytes32 cachedRoot);
+
+
+ mapping(bytes32 => bytes32) public merkleCache;
/// @notice Mapping of function selector to plugin address
mapping(bytes4 => address) public methods;
@@ -49,7 +55,8 @@ contract Vault is IVault, NFTReceiver {
function execute(
address _target,
bytes calldata _data,
- bytes32[] calldata _proof
+ bytes32[] calldata _proof,
+ bool checkCache
) external payable returns (bool success, bytes memory response) {
bytes4 selector;
assembly {
@@ -59,14 +66,44 @@ contract Vault is IVault, NFTReceiver {
// Generate leaf node by hashing module, target and function selector.
bytes32 leaf = keccak256(abi.encode(msg.sender, _target, selector));
// Check that the caller is either a module with permission to call or the owner.
- if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
- if (msg.sender != owner)
- revert NotAuthorized(msg.sender, _target, selector);
+ if(checkCache){
+ bytes32 cached = merkleCache[leaf];
+ if(cached != merkleRoot)
+ revert CacheFailed(address(this), leaf, cached);
+
+ }else{
+ if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
+ if (msg.sender != owner)
+ revert NotAuthorized(msg.sender, _target, selector);
+ }
}
(success, response) = _execute(_target, _data);
}
+ function cacheMerkleProof(
+ address user,
+ address _target,
+ bytes calldata _data,
+ bytes32[] calldata _proof
+ ) public{
+ bytes4 selector;
+ assembly {
+ selector := calldataload(_data.offset)
+ }
+
+ // Generate leaf node by hashing module, target and function selector.
+ bytes32 leaf = keccak256(abi.encode(user, _target, selector));
+ // Check that the caller is either a module with permission to call or the owner.
+ if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
+ if (user != owner)
+ revert NotAuthorized(user, _target, selector);
+ }
+
+ merkleCache[leaf] = merkleRoot;
+ emit Cache(address(this), leaf, merkleRoot);
+ }
+
Here's the gas diff when I cached the leafs of the Buyout module
Notice that most of the functions of Buyout.sol went down by 500-700 units per call, as expected
Execute went down by avg of 200 units, and up ~60 units in some cases (probably when not cached)
Disclaimer: Foundry might be considered each test as one tx, making the cache warm after the 1st call (therefore charging less gas than real world scenario). Further testing is needed (and there's no much time left for that till the end of the contest)
Table of Content
initForfunction to Vault to save gas inVaultFactorydeleteinstead of setting to zeroreuse loaded variable to get total supply
^ back to top ^
Instead of calling
IVaultRegistry(registry).totalSupply(_vault), which would load againtokenandidfrom storage, we could use thetokenandidwe've already loaded.Some variable can be immutable
^ back to top ^
This would save loading the variable from storage each time.
Add an
initForfunction to Vault to save gas inVaultFactory^ back to top ^
Instead of doing an init and then transferring ownership (transferOwner takes an extra
sstoreop), just add toVaultfunctionality that let's init for another address and use it inVaultFactory.Use
deleteinstead of setting to zero^ back to top ^
This doesn't seem to save gas at runtime, but does save a bit of deployment size
Deployment size and cost diff:
Don't transfer when amount is zero
^ back to top ^
Buyout.startdepositAmountmight be zero (in case that the proposer doesn't hold any fractions)Buyout.endtokenBalancemight be zero (if all fractions were bought) SinceIERC1155.safeTransferFromcosts about 21K gas units on avg (according to the gas report), it's worth checking before that the amount isn't zero.cache merkle proof-check
^ back to top ^
This one might need further checking and consideration, but caching merkle proof might be cheaper in the long run. Here's the math:
leafs > 4, current situation in testing), 3.6K for 4 layers (leafs > 8) and 4.5K for 5 layers (leafs > 16)Here's the suggested code change:
Here's the gas diff when I cached the leafs of the Buyout module
Buyout.solwent down by 500-700 units per call, as expectedDisclaimer: Foundry might be considered each test as one tx, making the cache warm after the 1st call (therefore charging less gas than real world scenario). Further testing is needed (and there's no much time left for that till the end of the contest)