ERTP Trusted User Inputs

[katelynsills]

Issue A: ERTP Trusted User Inputs

Location

agoric-sdk/packages/ERTP/src/paymentLedger.js

Line 138 in f6a87ba

payments.forEach(payment => paymentLedger.delete(payment));

Synopsis

ERTP’s internal reallocate function takes an array as input and calls Array.forEach and other standard array methods on it. However, objects passed in by the user should not be trusted. An object with the correctly named methods, but incorrect behavior, could be passed.

Impact

The reallocate function is not exposed publicly and the methods that do call it happen to copy the array. However, given access to this function, an attacker could create free money resulting in a loss of legitimate funds.

Preconditions

An attacker gains access to the reallocate function, or a method that calls reallocate uses the user-supplied payments array instead of copying it.

Technical Details

The methods that call reallocate use Promise.all on their input array. This uses the iterator interface to copy the array and resolves a new and safe array. However, if a user input were to be passed directly to reallocate and the forEach function misbehaved, it would be possible to duplicate payment objects. The reallocate function iterates over the payments and calculates the sum of their amounts. It then creates new payment objects with the same total amount, then calls forEach again to delete the original payment objects. If the payment.forEach method just did nothing on the second call, the payments would not be deleted, and they could be used to create new payment objects again. This would allow the caller to create free money. zcfSeat also assumes that forEach behaves correctly, but is called by methods that copy the array.

Suggested Remediation

We recommend importing trusted methods and passing user objects to them, instead of trusting functions on objects passed in by users. For inputs such as arrays, we suggest copying the data to new trustworthy arrays. A simple way to copy the array is to use a destructuring pattern combined with the spread operator ([...array]), which creates a new array that is a copy of the array elements.

We also recommend that the reallocate function perform this check on its payments argument because of the importance of the function.

Remediation

PR #3892 ensured that all user-provided inputs in ERTP were validated. In the case of a payment array, passStyleOf was used to ensure that the payment array was a copyArray, a valid, hardened, pass-by-copy array. Additionally, the internal reallocate function (renamed to moveAssets to distinguish it from reallocation in Zoe) also checks that the payment array is a copyArray as an additional layer of defense.

[katelynsills]

Closed in #3892