search

code-423n4 / 2022-06-connext-findings

1 stars 0 forks source link

QA Report #194

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Native MATIC has contract address on Polygon

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/SponsorVault.sol#L286-L300

Vulnerability details

Impact

MATIC transfers will revert

Proof of Concept

Native MATIC on Polygon has the contract address 0x0000000000000000000000000000000000001010, so when withdraw is called it will be treated as an ERC20 token. MATIC transfer requires that msg.value = value (https://polygonscan.com/address/0x0000000000000000000000000000000000001010#code L518). Since withdraw doesn't call transfer with a message value, the transfer will return false causing safeTransfer to revert.

Tools Used

Recommended Mitigation Steps

Create exception for native MATIC in function

jakekidd commented 2 years ago

Confirming issue. Might be best to have the native token address be a set value in an initializer, but the most gas efficient method would be to set it as a constant.

jakekidd commented 2 years ago

Amending this evaluation: according to the design of the application and the intended use-case, any/all references to native token should be address(0). Simply passing in address(0) as the argument for token is sufficient to indicate usage of the native token.

If anything, this is a QA issue.

LayneHaber commented 2 years ago

^^ correct, this is a convention (already being used in production on MATIC). The main concern is the treatment, which means on the frontend all native assets should use the address(0) convention.

The matic team is familiar with the convention already :) (and we did run into this issue in production with nxtp v0)

0xleastwood commented 2 years ago

As per the sponsor's comments, downgrading this to QA.