stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Report error instead of hitting arithmetic underflow in `delegate-stack-increase`

Open obycode opened this issue 1 year ago • 0 comments

In pox-4, if you call delegate-stack-increase when not delegated, instead of returning a nice error, the transaction will hit a runtime arithmetic underflow. We should add an assertion to check for this case and report a nice error before reaching this subtraction.

Example test case can be found here: https://github.com/stacks-network/stacks-core/blob/c9cec42bc848bbd134f8dff8f2a0140e608b18fd/contrib/boot-contracts-unit-tests/tests/pool-delegate.test.ts#L1032-L1048

The beginning of this function is:

(define-public (delegate-stack-increase
                    (stacker principal)
                    (pox-addr { version: (buff 1), hashbytes: (buff 32) })
                    (increase-by uint))
    (let ((stacker-info (stx-account stacker))
          (existing-lock (get locked stacker-info))
          (available-stx (get unlocked stacker-info))
          (unlock-height (get unlock-height stacker-info)))                 ;; <-- this is 0

     ;; must be called with positive `increase-by`
     (asserts! (>= increase-by u1)
               (err ERR_STACKING_INVALID_AMOUNT))

     (let ((unlock-in-cycle (burn-height-to-reward-cycle unlock-height))    ;; <-- this is 0
           (cur-cycle (current-pox-reward-cycle))                           ;; <-- this is 0
           (first-increase-cycle (+ cur-cycle u1))                          ;; <-- this is 1
           (last-increase-cycle (- unlock-in-cycle u1))                     ;; <-- underflow! (0 - 1)
           (cycle-count (try! (if (<= first-increase-cycle last-increase-cycle)
                                  (ok (+ u1 (- last-increase-cycle first-increase-cycle)))
                                  (err ERR_STACKING_INVALID_LOCK_PERIOD))))
           (new-total-locked (+ increase-by existing-lock))
           (stacker-state
                (unwrap! (map-get? stacking-state { stacker: stacker })
                 (err ERR_STACK_INCREASE_NOT_LOCKED))))

The problem arises from the fact that the unlock-height binding is going to be 0. See comments above to follow how the underflow is reached.

The right solution is probably just to add:

      (asserts! (> unlock-height u0) ERR_STACK_INCREASE_NOT_LOCKED)

obycode avatar Apr 25 '24 20:04 obycode