Skip to content

refactor timelocks 2#842

Closed
MathisGD wants to merge 13 commits intomainfrom
refactor/timelocks-use-vault-mapping
Closed

refactor timelocks 2#842
MathisGD wants to merge 13 commits intomainfrom
refactor/timelocks-use-vault-mapping

Conversation

@MathisGD
Copy link
Copy Markdown
Contributor

@MathisGD MathisGD commented Dec 2, 2025

alternative to #839

major drawbacks:

  • the adapter needs to be sentinel
  • there can be a selector clash

@MathisGD MathisGD self-assigned this Dec 2, 2025
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 66 to 71
function timelocked() internal {
require(executableAt[msg.data] != 0, DataNotTimelocked());
require(block.timestamp >= executableAt[msg.data], TimelockNotExpired());
executableAt[msg.data] = 0;
uint256 executableAt = IVaultV2(parentVault).executableAt(msg.data);
require(executableAt != 0, DataNotTimelocked());
require(block.timestamp >= executableAt, TimelockNotExpired());
IVaultV2(parentVault).revoke(msg.data);
emit Accept(bytes4(msg.data), msg.data);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid unauthorized revoke in timelocked adapter flow

Timelocked adapter calls now clear the vault timelock via IVaultV2(parentVault).revoke(msg.data) (lines 66‑70), but VaultV2.revoke only allows the curator or a sentinel to call it (see VaultV2.sol lines 362‑364). When the curator executes setSkimRecipient or burnShares after submitting them through the vault, the adapter performs this revoke as itself and will hit Unauthorized() unless the adapter was pre-added as a sentinel (not the default). That bricks all timelocked adapter actions in production unless an extra sentinel configuration step is done.

Useful? React with 👍 / 👎.

Comment on lines +69 to 71
require(block.timestamp >= executableAt, TimelockNotExpired());
IVaultV2(parentVault).revoke(msg.data);
emit Accept(bytes4(msg.data), msg.data);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here it also emits the Revoke event on the vault, but it is actually accepted. It could be confusing, or difficult to index (maybe that's ok because Revoke logs the sender, which is the adapter in this case)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree that's misleading. but on the other side you gain that you don't need to index the adapter's storage

@MathisGD MathisGD changed the title feat: timelocks reuse the vault executableAt refactor timelocks 2 Dec 2, 2025
@MathisGD MathisGD changed the base branch from refactor/timelocks to main December 2, 2025 12:28
@MathisGD
Copy link
Copy Markdown
Contributor Author

MathisGD commented Dec 2, 2025

replaced by #839

@MathisGD MathisGD closed this Dec 2, 2025
@MathisGD MathisGD deleted the refactor/timelocks-use-vault-mapping branch December 2, 2025 14:35
@MathisGD
Copy link
Copy Markdown
Contributor Author

MathisGD commented Dec 2, 2025

see this thread

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.

2 participants