Improper Upper Bound Definition on the Fee

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L456

Vulnerability details

Impact

The newSwapFee does not have any upper or lower bounds. Values that are too large will lead to reversions in several critical functions or the platform user will lost all funds when paying the fee. If the authorization sets to fee as %100, the exchange owner can steal funds.

Proof of Concept

1. Navigate to the following contract : https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L456

Tools Used

Code Review

Recommended Mitigation Steps

Consider defining upper and lower bounds on the newSwapFee variable.

[jakekidd]

"Exchange ownership" in this case refers to the Owner role of the contract. This role ought to be delegated to DAO governance in the future, but by design relies on the Owner choosing reasonable settings in the meantime. Regardless, it would be responsible to have a min/max cap on this value as a constant. For this reason, I'm confirming the issue but advising it be de-escalated.

[jakekidd]

The setSwapFee actually used in StableSwapFacet belongs to SwapUtils, not StableSwap.sol

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1084

Here, we enforce that the fee is <= MAX_SWAP_FEE

  // Max swap fee is 1% or 100bps of each swap
  uint256 internal constant MAX_SWAP_FEE = 10**8;

[0xleastwood]

This is already enforced.