Critical vulnerabilities in Jetton with Governance contract

yumik229 commented 8 months ago

Specific code sections with vulnerabilities in the Jetton with Governance smart contract:

1. Admin key vulnerability:

The JettonMinter contract defines several functions that can only be called by the admin, including: transferFrom: Allows the admin to transfer jettons from any user's wallet. burn: Allows the admin to burn jettons from any user's wallet. setStatus: Allows the admin to lock/unlock any user's wallet. upgrade: Allows the admin to change the jetton-minter code and data. These functions rely on the require_owner modifier, which checks if the message sender is the owner (admin) of the contract:

modifier require_owner() {
    require(msg.sender == owner, 100);
    _;
}

If the admin key is compromised, an attacker could use these functions to steal or burn user jettons, or even upload malicious code through the upgrade function.

2. Unrestricted upgrade function:

The upgrade function in the JettonMinter contract allows the admin to update the contract code and data:

() upgrade(cell code, cell data) require_owner {
    set_code(code);
    set_data(data);
}

This function doesn't perform any validation on the provided code and data. This means a malicious admin could upload code that performs unauthorized actions, such as stealing jettons or granting themselves additional privileges. Mitigation: Implement validation logic within the upgrade function to ensure only authorized code and data can be uploaded. Consider using a multi-signature approval process for upgrades, requiring multiple parties to agree before a new code or data is deployed. These vulnerabilities highlight the importance of securing the admin key and implementing proper validation checks in critical functions like upgrade. It's crucial to address these vulnerabilities to protect user funds and maintain the integrity of the Jetton with Governance smart contract.

Modified code to prevent vulnerabilities: Here's how you can modify the code to address the vulnerabilities mentioned:

1. Replacing the admin key with a multi-signature wallet:

a) Change the owner variable type:

In the JettonMinter contract, change the type of the owner variable from address to cell:

cell owner;

This will allow you to store a reference to a multi-signature wallet contract instead of a single address.

b) Modify the require_owner modifier:

Replace the existing require_owner modifier with the following code:

modifier require_owner() {
    // Replace this with the logic to verify a valid multi-signature from the wallet contract
    require(multisig_wallet.verify_signature(msg.body), 100);
    _;
}

This code assumes you have a multisig_wallet variable that references the multi-signature wallet contract and a verify_signature function within that contract to validate signatures. You need to implement the specific logic for verifying multi-signatures based on your chosen wallet contract.

2. Implementing code and data validation for the upgrade function:

a) Add checks within the upgrade function:

Modify the upgrade function to include validation checks before setting the new code and data:

() upgrade(cell code, cell data) require_owner {
    // Verify code and data integrity and authenticity
    require(hash(code) == pre_approved_code_hash, 101);
    require(hash(data) == pre_approved_data_hash, 102);

    set_code(code);
    set_data(data);
}

This code assumes you have pre-calculated and stored the approved hashes (pre_approved_code_hash and pre_approved_data_hash) for the code and data. You can use any secure hashing algorithm like SHA-256 for this purpose.

b) Alternatively, create a separate upgrade approval process:

Instead of directly deploying the code and data within the upgrade function, you can implement a separate process where the upgrade package is first submitted for review and approval by designated parties. Once approved, the upgrade function can be called with the pre-approved code and data. These modifications will significantly improve the security of the Jetton with Governance smart contract by requiring multi-signature authorization for critical actions and ensuring that only validated code and data can be used for upgrades. Remember to adapt these changes to your specific multi-signature wallet contract and upgrade approval process. It's also recommended to conduct thorough testing and consider formal verification to ensure the modified contract is secure and functions as intended.

ProgramCrafter commented 8 months ago

Both of these considerations were already noted in README. Also, your code analysis may be imprecise (seems that it uses LLM that automatically created something named "modifer", which does not exist in TON).