hats-finance / illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf

0 stars 1 forks source link

check address for zero #11

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0x9ac0c34967e1a158136b149cccf233ff3f1dbdc18da5aa6d842cdaf93b47fdeb Severity: low

Description: Description\ In constructor is set prover address, but it doesnt check for zero address

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File https://github.com/hats-finance/illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/blob/db34511e17fdf281aacef4267c300431d3ac12d7/packages/contracts/contracts/illuminex/xengine/chains/btc/wallet/BitcoinAbstractWallet.sol#L30

Recommendation

should check _prover != address(0)

rotcivegaf commented 4 months ago

Non issue, at most informational

Jelev123 commented 4 months ago

In Solidity, constructors often take address parameters to initialize important components of a contract, such as owner or linked contracts. However, without a check, there's a risk that an address parameter could be mistakenly set to the zero address (0x0). This could occur due to a mistake or oversight during contract deployment. A zero address in a crucial role can cause serious issues, as it cannot perform actions like a normal address, and any funds sent to it are irretrievable. Therefore, it's crucial to include a zero address check in constructors to prevent such potential problems. If a zero address is detected, the constructor should revert the transaction. Its a similar issue in Cantina-Euler which is valid low @rotcivegaf

Jelev123 commented 4 months ago

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

party-for-illuminati commented 4 months ago

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

I don't get it. What if somehow owner will set it to address(1), does this mean we should check for address(1) as well?

Jelev123 commented 4 months ago

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

I don't get it. What if somehow owner will set it to address(1), does this mean we should check for address(1) as well?

Addresses like address(1) could be intentionally assigned for specific purposes and do not inherently carry the same risks as the zero address. While it might seem like a minor issue, including a zero address check in constructors is a simple and effective measure to enhance the contract's reliability and security. It's not about adding checks for every possible incorrect address but rather addressing a specific and universally recognized risk.

party-for-illuminati commented 4 months ago

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

I don't get it. What if somehow owner will set it to address(1), does this mean we should check for address(1) as well?

Addresses like address(1) could be intentionally assigned for specific purposes and do not inherently carry the same risks as the zero address. While it might seem like a minor issue, including a zero address check in constructors is a simple and effective measure to enhance the contract's reliability and security. It's not about adding checks for every possible incorrect address but rather addressing a specific and universally recognized risk.

image
Jelev123 commented 4 months ago

this issue is from Cantina-Euler and it is accepted. Its a valid low