0xbranded - Hardcoded Value Breaks Core Contract Functionality

[sherlock-admin4]

0xbranded

Medium

Hardcoded Value Breaks Core Contract Functionality

Summary

The use of a hardcoded value for the pool id in apy.vy proxied calls makes every other pool in the system inaccessible.

Vulnerability Detail

The comments indicate that apy.vy is the entry-point contract, which proxy's calls to core. However, each call made to core.vy includes a hardcoded value of 1 for the pool id:

  return self.CORE.mint(1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)
  return self.CORE.burn(1, base_token, quote_token, lp_token, lp_amt, ctx)
  return self.CORE.open(1, base_token, quote_token, long, collateral0, leverage, ctx)
  return self.CORE.close(1, base_token, quote_token, position_id, ctx)
  return self.CORE.liquidate(1, base_token, quote_token, position_id, ctx)

Within each of these functions in core.vy, the following checks are enforced:

  pool       : PoolState = self.POOLS.lookup(id)
...
  assert pool.base_token  == base_token , ERR_PRECONDITIONS
  assert pool.quote_token == quote_token, ERR_PRECONDITIONS

However the pools contract is designed to support multiple pools with differing pool id:

def fresh(
  symbol     : String[65],
  base_token : address,
  quote_token: address,
  lp_token   : address) -> PoolState:
  self._INTERNAL()
  return self.insert(PoolState({
    id               : self.next_pool_id(),
...

def next_pool_id() -> uint256:
  id : uint256 = self.POOL_ID
  nxt: uint256 = id + 1
  self.POOL_ID = nxt
  return nxt

Thus, any user action with core.vy can interact only with this first pool and all pools with an id differing from 1 will be inaccessible.

Impact

Core contract functionality is broken, leading to a DoS (which doesn't compromise funds, since pools cannot be interacted with to begin with).

Code Snippet

Tool used

Manual Review

Recommendation

Add an id: uint256 parameter to the mentioned functions of api.vy, and forward this value to the calls of core.vy, rather than hardcoding 1.

[spacegliderrrr]

Escalate.

This is intended. An instance of each contract will be deployed for each supported token pair. This has been mentioned in the Readme, the discord server and noted in the code comments.

[sherlock-admin3]

Escalate.

This is intended. An instance of each contract will be deployed for each supported token pair. This has been mentioned in the Readme, the discord server and noted in the code comments.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[msheikhattari]

Agreed - can close 👍. The design of the contracts seems to suggest multiple pools will be supported, but no issues as per the Readme.

[WangSecurity]

Agree with the escalation, this is indeed the intended design of the protocol. Planning to accept the escalation and invalidate the issue.