Skip to content

Conversation

@Shvandre
Copy link
Collaborator

@Shvandre Shvandre commented Jul 8, 2025

Big thanks to @Gosunov for finding that security issue

Copilot AI review requested due to automatic review settings July 8, 2025 12:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the TON Vault contract to remove the admin address from its initialization and addresses a security issue by encapsulating forwarded user payloads in a dedicated message type.

  • Removed admin parameter from TonVault.fromInit and contract definition
  • Introduced PayoutFromTonVault message in the interface and updated handlePayout to use it
  • Updated environment helpers, deployment script, and tests to match the new initialization and cover payload forwarding

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sources/utils/environment.ts Updated createTonVault to call fromInit() without args
sources/tests/ton-vault.spec.ts Added payload-forwarding test and adjusted imports
sources/scripts/deploy.ts Changed deployment to use fromInit() without admin arg
sources/contracts/vaults/vault-interface.tact Added PayoutFromTonVault message definition
sources/contracts/vaults/ton-vault.tact Removed admin in contract signature and updated payout logic
Comments suppressed due to low confidence (2)

sources/contracts/vaults/vault-interface.tact:54

  • [nitpick] Consider adding a brief comment above this message definition to explain the purpose of PayoutFromTonVault and its body field.
message(0x2d8b123a) PayoutFromTonVault {

sources/contracts/vaults/ton-vault.tact:42

  • [nitpick] The indentation for the PayoutFromTonVault body block is slightly different from surrounding code; aligning it with existing style would improve readability.
            body: PayoutFromTonVault {

Copy link
Collaborator

@Kaladin13 Kaladin13 left a comment

Choose a reason for hiding this comment

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

Great fix

@Kaladin13 Kaladin13 merged commit 3f9622a into main Jul 8, 2025
1 check passed
@Kaladin13 Kaladin13 deleted the security-fix branch July 8, 2025 13:43
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