QA Report

[code423n4]

safeApprove() for may revert

Lines of code

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

Vulnerability details

Impact

safeApprove function prevents changing an allowance between non-zero values to mitigate a possible front-running attack. It reverts if that is the case. Instead, the safeIncreaseAllowance and safeDecreaseAllowance functions should be used. Comment from the OZ library for this function: “// safeApprove should only be called when setting an initial allowance, // or when resetting it to zero. To increase and decrease it, use // 'safeIncreaseAllowance' and ‘safeDecreaseAllowance'"

Proof of Concept

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6842518b1b71fac9a21c7d94ec521992cff266b5/contracts/token/ERC20/utils/SafeERC20.sol#L44-L57

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use safeIncreaseAllowance() function instead of safeApprove().

[ecmendenhall]

Duplicate of #154

[jakekidd]

Closed as duplicate of #154

[0xleastwood]

No mention of what impact this will have on the protocol's users and their respective funds. This isn't simply a revert issue, this legitimately breaks the flow of bridge transfers under certain token implementations.

[0xleastwood]

Downgrading to QA.

[0xleastwood]

However, the warden submitted multiple QA reports. As such, this will not be judged.