search

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

1 stars 0 forks source link

Deprecated safeApprove() function #242

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347

Vulnerability details

Impact

Using this deprecated function can lead to unintended reverts and potentially the locking of funds and also frontrunings.

Proof of Concept

A deeper discussion on the deprecation of this function is in OZ issue #2219. The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

Tools Used

Manual review

Recommended Mitigation Steps

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead: In AssetLogic.sol, 

347:         SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);
ecmendenhall commented 2 years ago

Duplicate of #154

jakekidd commented 2 years ago

While this is a more generic description of the safeApprove method having deprecated support from OZ, I think any sufficient handling of #154 will preclude this issue being handled. As such, marking as duplicate of #154

0xleastwood commented 1 year ago

This issue does not highlight the impact in any way. As such, I will downgrade this to QA. Its important to highlight under what circumstances dust approvals can break the flow of bridge transfers.

0xleastwood commented 1 year ago

Merging with #247.