Omniscia Bonq Audit

arbitrage-pool Manual Review Findings

arbitrage-pool Manual Review Findings

ARB-01M: Inexplicable Capability of Re-Invocation

Description:

The router of the system can be arbitrarily set by its owner.

Example:

contracts/arbitrage-pool.sol
128/// @dev sets the address of the Router to enable arbitrage
129/// @param  _router address of the contract
130function setRouter(address _router) public onlyOwner {
131  router = IRouter(_router);
132}

Recommendation:

We advise this trait of the system to be re-evaluated and potentially omitted as it serves no real use-case purpose as router addresses of DEXes do not change.

Alleviation:

The Bonq Protocol team has removed the setRouter function.

ARB-02M: Inexistent Re-Entrancy Protection

Description:

It is possible to sabotage the contract by artificially inflating the depositsAndRewards value via a re-entrancy attack on a malicious token added to the trading system of the router.

Impact:

It is currently possible to make a significant portion of funds in the contract unredeemable by exploiting a re-entrancy attack in the router's trade sequence to re-invoke the arbitrage function and measure the same delta in assets multiple times.

Example:

contracts/arbitrage-pool.sol
89/// @dev to do arbitrage
90/// @param  _collateralToken collateral token address
91/// @param  _amountIn start amount
92/// @param  _path calldata[]
93/// @param  _deadline deadline time
94function arbitrage(
95  address _collateralToken,
96  uint256 _amountIn,
97  address[] calldata _path,
98  uint256 _deadline
99) public {
100  require(_path[0] == _collateralToken, "92852 must start with collateralToken");
101  require(_path[_path.length - 1] == _collateralToken, "92852 must end with _collateralToken");
102  // if the deadline was not set it is set to NOW - as the swap will happen in the same block it will be soon enough
103  _deadline = _deadline > 0 ? _deadline : block.timestamp;
104  IERC20 collateralToken = IERC20(_collateralToken);
105  uint256 startBalance = collateralToken.balanceOf(address(this));
106  // the swap must yield at least 1 coin (in ETH parlance: 1 Wei) more than what was put in and the TX has 10 minutes to execute
107  router.swapExactTokensForTokens(_amountIn, _amountIn + 1, _path, address(this), _deadline);
108  uint256 amountOut = collateralToken.balanceOf(address(this)) - startBalance;
109  depositsAndRewards[_collateralToken] += amountOut;
110

111  emit Arbitrage(_collateralToken, _path, _amountIn, amountOut);
112}

Recommendation:

We advise a nonReentrant modifier to be introduced throughout the contract to disallow this kind of attack as otherwise the balance measurements utilized are unsafe.

Alleviation:

The Bonq Protocol team has added the nonReentrant modifier to the arbitrage function.

ARB-03M: Inexistent Validation of Existing AP Token Entry

Description:

The addToken function does not check whether a particular token has already been added thereby permitting tokens deposited to be completely at risk.

Impact:

It is currently possible to siphon all deposited funds by overwriting the AP token of a collateral asset, making a miniscule deposit and redeeming with the old depositsAndRewards value in a single transaction.

Example:

contracts/arbitrage-pool.sol
114/// @dev to add new supported collateral by deploying APtoken for it
115/// @param  _collateralToken collateral to be added
116function addToken(address _collateralToken) public {
117  IERC20Metadata collateralToken = IERC20Metadata(_collateralToken);
118  string memory apTokenName = string(abi.encodePacked("APToken for ", collateralToken.name()));
119  string memory apTokenSymbol = string(abi.encodePacked("AP", collateralToken.symbol()));
120  APToken newAPtoken = new APToken(apTokenName, apTokenSymbol, _collateralToken);
121  address newAPtokenAddress = address(newAPtoken);
122  collateralToAPToken[_collateralToken] = IMintableToken(newAPtokenAddress);
123  collateralToken.approve(address(router), MAX_INT);
124

125  emit APtokenDeployed(_collateralToken, newAPtokenAddress);
126}

Recommendation:

We advise the system to properly evaluate whether the collateralToAPToken entry is non-zero as in such a case the function should fatally fail.

Alleviation:

The Bonq Protocol team has introduced sufficient checks validating whether the collateralToAPToken entry has already been set.

Prev

trove.sol (TRO-S)

Next

external-price-feed.sol (EXT-M)

On this page