Oyster Secret Storage Implementation #14
Conversation
vg-27
force-pushed
the
rajat/secret-store
branch
from
54d5f5d to
738a5e0
Compare
|
Impl looks, good, can add UTs and scripts for deployment. |
| wallets = signers.map((_, idx) => walletForIndex(idx)); | ||
|
|
||
| token = addrs[1], | ||
| noOfNodesToSelect = 3, |
| await expect(secretManager.acknowledgeStoreFailed(secretId)) | ||
| .to.be.revertedWithCustomError(secretManager, "SecretManagerAcknowledgedAlready"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
add case where at least one secret store has acknowledged but not others.
| let secretId = 1; | ||
| await expect(secretManager.connect(signers[1]).terminateSecret(secretId)) | ||
| .to.be.revertedWithCustomError(secretManager, "SecretManagerNotUserStoreOwner"); | ||
| }); |
There was a problem hiding this comment.
few more cases:
- termination after termination
- termination after end timestamp
- combination of termination and acknowledgement time period
|
|
||
| //-------------------------------- TeeManager start ----------------------------------// | ||
|
|
||
| modifier onlySecretStore() { |
There was a problem hiding this comment.
Current approach is better as we already have the SECRET_STORE storage var. With onlyRole, we will have to do an extra transaction to grant the role.
There was a problem hiding this comment.
what's the pros of using onlyRole that we are missing here? Any security concerns? Also, you can set the ROLE during initialization of contract.
There was a problem hiding this comment.
TeeManager contract will be deployed first and then the Executors and SecretStore contracts. So, we cannot assign the role during initialization. I have made setter functions for these two contracts.
We can grant the roles within these setter functions but there isn't any security threat with the existing approach.
There was a problem hiding this comment.
Also, just in case we have to update the Executors or SecretStore contract, first we will have to revoke the role and then grant it to the new contract, so another overhead.
|
|
||
| //-------------------------------- TeeManager start ----------------------------------// | ||
|
|
||
| modifier onlySecretStore() { |
There was a problem hiding this comment.
what's the pros of using onlyRole that we are missing here? Any security concerns? Also, you can set the ROLE during initialization of contract.
| @@ -378,7 +378,9 @@ contract Executors is | |||
| uint256[] memory stakeAmounts = TEE_MANAGER.getTeeNodesStake(_enclaveAddresses); | |||
| uint256 len = _enclaveAddresses.length; | |||
| for (uint256 index = 0; index < len; index++) { | |||
| _insert_unchecked(_env, _enclaveAddresses[index], uint64(stakeAmounts[index] / STAKE_ADJUSTMENT_FACTOR)); | |||
| address enclaveAddress = _enclaveAddresses[index]; | |||
| if (executors[enclaveAddress].activeJobs < executors[enclaveAddress].jobCapacity) | |||
There was a problem hiding this comment.
make sure whoever is calling this method, doesn't put this check of active jobs, otherwise it would be redundant.
| @@ -21,8 +21,30 @@ contract JobsUser { | |||
|
|
|||
| event FailedCallback(uint256 indexed jobId, uint256 slashedAmount); | |||
|
|
|||
| // function createJob( | |||
There was a problem hiding this comment.
it is still being used in the serverless-v2 contracts, so let's keep it for now.
| address _enclaveAddress, | ||
| uint256 _storageOccupied | ||
| ) internal { | ||
| secretStores[_enclaveAddress].storageOccupied -= _storageOccupied; |
There was a problem hiding this comment.
we have to make sure storageOccupied get to the value zero before deregistration and remove stake. i.e. no secrets assigned.
There was a problem hiding this comment.
already being checked in the respective functions
| ) as unknown as AttestationVerifier; | ||
|
|
||
| const USDCoin = await ethers.getContractFactory("USDCoin"); | ||
| let usdcToken = await upgrades.deployProxy( |
| }); | ||
|
|
||
| // drain then deregister with no occupied storage | ||
| it('can deregister tee node without occupied storage', async function () { |
There was a problem hiding this comment.
| it('can deregister tee node without occupied storage', async function () { | |
| it('can deregister tee node without occupied storage and assigned jobs', async function () { |
| }); | ||
|
|
||
| // drain then deregister failed with active jobs != 0 | ||
| it('cannot deregister secret store with occupied storage', async function () { |
There was a problem hiding this comment.
update the comment, there is no jobs assigned.
| // select nodes | ||
| await secretManager.selectStores(env, 1, 100); | ||
| // drain | ||
| await teeManager.connect(signers[1]).drainTeeNode(addrs[15]); |
There was a problem hiding this comment.
relinquish will happen as part of drain, deregister should pass through after this.
There was a problem hiding this comment.
Here we haven't ack the secret, so it still occupies storage. Only after ack, secret is added to the ackSecretIds list, and then removed during draining.
| // select nodes | ||
| await secretManager.selectStores(1, 1, 100); | ||
| // drain | ||
| await teeManager.connect(signers[1]).drainTeeNode(addrs[15]); |
There was a problem hiding this comment.
drain should have relinquished the hold of secret, please check.
There was a problem hiding this comment.
secret is unacknowledged
| .to.be.revertedWithCustomError(teeManager, "TeeManagerInvalidEnclaveOwner"); | ||
| }); | ||
|
|
||
| it('cannot unstake with occupied storage after draining started', async function () { |
| .to.emit(teeManager, "TeeNodeRevived").withArgs(addrs[15]); | ||
| }); | ||
|
|
||
| // it("can revive tee node after draining", async function () { |
There was a problem hiding this comment.
what's the reason for this commented test function? what's the todo here?
| takeSnapshotBeforeAndAfterEveryTest(async () => { }); | ||
|
|
||
| it("can slash store", async function () { | ||
| let stakeAmount = 10n ** 20n, |
There was a problem hiding this comment.
can you set stake amount to max unit256 and test?
There was a problem hiding this comment.
The max POND supply is 10 billion (10^28). Updated the test for this value.
| let wallet = ethers.HDNodeWallet.fromPhrase("test test test test test test test test test test test junk", undefined, "m/44'/60'/0'/0/" + idx.toString()); | ||
|
|
||
| return new Wallet(wallet.privateKey); | ||
| } |
| let storageCapacity = 1e9, | ||
| stakeAmount = 10n ** 19n; | ||
| for (let index = 0; index < 2; index++) { | ||
| await teeManager.registerSecretStore( |
There was a problem hiding this comment.
Ideally, mocks should have the exact same function signature as that of the original contract. Like, registerSecretStore is not a function TeeManager contract.
|
|
||
| takeSnapshotBeforeAndAfterEveryTest(async () => { }); | ||
|
|
||
| it("can stake", async function () { |
There was a problem hiding this comment.
we should check the state of tree after stake is added or removed. Condition of minimum stake.
| it('cannot drain secret store twice consecutively', async function () { | ||
| let env = 1; | ||
| await teeManager.drainSecretStore(addrs[15], env, addrs[0]); | ||
| await expect(secretStore.connect(signers[1]).drainSecretStore(addrs[15], env, addrs[0])) |
There was a problem hiding this comment.
can you validate the reason of revert here? invalid account is trying to call the drain on secret store.
There was a problem hiding this comment.
Yeah, it was incorrect. Btw, we cannot test this case here as it is specific to TeeManager contract, where we already have a test for it. So removed this test from SecretStore and Executors.
| }); | ||
| }); | ||
|
|
||
| describe("SecretStore - Drain/Revive secret store", function () { |
| await expect(secretManager.markAliveUpdate(addrs[15], currentCheckTimestamp, 500, addrs[2])) | ||
| .to.not.be.reverted; | ||
|
|
||
| expect(await secretStore.getSecretStoreLastAliveTimestamp(addrs[15])).to.eq(currentCheckTimestamp); |
There was a problem hiding this comment.
can you validate that slashing has happened?
| .to.not.be.reverted; | ||
|
|
||
| expect(await secretStore.getSecretStoreDeadTimestamp(addrs[15])).to.eq(currentCheckTimestamp); | ||
| expect(await secretStore.getStoreAckSecretIds(addrs[15])).to.deep.eq([]); |
There was a problem hiding this comment.
verify that slashing is happening
| let wallet = ethers.HDNodeWallet.fromPhrase("test test test test test test test test test test test junk", undefined, "m/44'/60'/0'/0/" + idx.toString()); | ||
|
|
||
| return new Wallet(wallet.privateKey); | ||
| } |
Doc - https://docs.google.com/document/d/1DNkw2BPdrYesIp1z_q31amENeruZiuZSfchC7YXQr1w/edit#heading=h.kxzyz8ao5ap9