Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
obycode opened this issue Apr 25, 2024 · 0 comments
Labels

Comments

@obycode
Copy link
Contributor

obycode commented Apr 25, 2024

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:

it("cannot be called if not delegated", () => {
const account = stackers[0];
const minAmount = getStackingMinimum();
const amount = minAmount * 2n;
const maxAmount = minAmount * 4n;
// Arithmetic underflow is not caught gracefully, so this triggers a runtime error.
// Preferably, it would return a `ERR_STACKING_NOT_DELEGATED` error.
expect(() =>
delegateStackIncrease(
account.stxAddress,
account.btcAddr,
maxAmount - amount,
address2
)
).toThrow();
});

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 obycode added the PoX-5 label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Status: 🆕 New
Development

No branches or pull requests

1 participant