// TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
// Example: https://github.com/aave/aave-v3-periphery/blob/ca184e5278bcbc10d28c3dbbc604041d7cfac50b/contracts/adapters/paraswap/ParaSwapRepayAdapter.sol#L138-L140
SafeERC20.safeIncreaseAllowance(IERC20(adopted), s.aavePool, totalRepayAmount);
// TODO: do we need to keep this
bytes32 details = action.detailsHash();
IBridgeToken(token).setDetailsHash(details);
Recommended Mitigation Steps
Consider choosing and implementing the method to address the approval issue, i.e. either do two step 0 -> amount approval or try-catch, before release.
It's advised to remove TODO comments in either way from production version of the code.
2. Incomplete description of AssetLogic's _swapAsset function (low)
_swapAsset() function description omits _slippageTol argument:
/**
* @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
* @dev Will not swap if the asset passed in is the adopted asset
* @param _canonicalId - The canonical token id
* @param _assetIn - The address of the from asset
* @param _assetOut - The address of the to asset
* @param _amount - The amount of the local asset to swap
* @return The amount of assetOut
* @return The address of assetOut
*/
function _swapAsset(
bytes32 _canonicalId,
address _assetIn,
address _assetOut,
uint256 _amount,
uint256 _slippageTol
) internal returns (uint256, address) {
Recommended Mitigation Steps
Add the description, for example:
/**
* @param _amount - The amount of the local asset to swap
* @param _slippageTol - Slippage tolerance with LIQUIDITY_FEE_DENOMINATOR decimals
* @return The amount of assetOut
* @return The address of assetOut
*/
3. Incomplete description of AssetLogic's _swapAssetOut function (low)
There is a typo and success return value is omitted:
/**
* @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
* @dev Will not swap if the asset passed in is the adopted asset
* @param _canonicalId - The canonical token id
* @param _assetIn - The address of the from asset
* @param _assetOut - The address of the to asset
* @param _amountOut - The amount of the _assetOut to swap
* @return The amount of assetIn
* @return The address of assetOut
*/
function _swapAssetOut(
bytes32 _canonicalId,
address _assetIn,
address _assetOut,
uint256 _amountOut,
uint256 _maxIn
)
Recommended Mitigation Steps
Consider updating to:
* @notice Swaps assetIn to assetOut using the stored stable swap or internal swap pool
...
* @return Swap success flag
* @return The amount of assetIn
...
/**
* @notice Emitted when the wrapper variable is updated
* @param oldWrapper - The wrapper old value
* @param newWrapper - The wrapper new value
* @param caller - The account that called the function
*/
event WrapperUpdated(address oldWrapper, address newWrapper, address caller);
/**
* @notice Emitted when the tokenRegistry variable is updated
* @param oldTokenRegistry - The tokenRegistry old value
* @param newTokenRegistry - The tokenRegistry new value
* @param caller - The account that called the function
*/
event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);
/**
* @notice Emitted when a new stable-swap AMM is added for the local <> adopted token
* @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
* @param domain - The domain of the canonical token for the local <> adopted amm
* @param swapPool - The address of the AMM
* @param caller - The account that called the function
*/
event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);
/**
* @notice Emitted when a new asset is added
* @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
* @param domain - The domain of the canonical token for the local <> adopted amm
* @param adoptedAsset - The address of the adopted (user-expected) asset
* @param supportedAsset - The address of the whitelisted asset. If the native asset is to be whitelisted,
* the address of the wrapped version will be stored
* @param caller - The account that called the function
*/
event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);
/**
* @notice Emitted when an asset is removed from whitelists
* @param canonicalId - The canonical identifier of the token removed
* @param caller - The account that called the function
*/
event AssetRemoved(bytes32 canonicalId, address caller);
Recommended Mitigation Steps
Consider adding indexes to ids and addresses in the all important events to improve their usability
1. Unpatched token approval issue in BridgeFacet, open TODOs (low)
Proof of Concept
Race condition issue isn't addressed, open TODO:
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1027-L1029
Another TODO comment:
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L579-L581
Recommended Mitigation Steps
Consider choosing and implementing the method to address the approval issue, i.e. either do two step 0 -> amount approval or try-catch, before release.
It's advised to remove TODO comments in either way from production version of the code.
2. Incomplete description of AssetLogic's _swapAsset function (low)
_swapAsset() function description omits _slippageTol argument:
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L252-L268
Recommended Mitigation Steps
Add the description, for example:
3. Incomplete description of AssetLogic's _swapAssetOut function (low)
There is a typo and success return value is omitted:
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L294-L310
Recommended Mitigation Steps
Consider updating to:
4. AssetFacet's events aren't indexed (non-critical)
Filtering on unindexed events is disabled, which makes it harder to programmatically use and analyse the system.
Proof of Concept
AssetFacet's events don't have any indices:
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/AssetFacet.sol#L21-L62
Recommended Mitigation Steps
Consider adding indexes to ids and addresses in the all important events to improve their usability