fix: prevent invalid transactions being submitted with osnap plugin (s…

…napshot-labs#4552) * disable publish if osnap tx invalid * set state upstream even if invalid, better input logic * don't allow empty transaction array * clear form when changing type * clone safe on mount * better validation, state setting for nft trasnfer & token transfer * keep validation logic in components, emit isValid flag * check contract interaction params * enforce positive value for sending tokens * remove top level valid flag * handle value validation * set invalid method params for validation * validate raw tx inputs * disable publish if osnap tx invalid * set state upstream even if invalid, better input logic * don't allow empty transaction array * clear form when changing type * clone safe on mount * better validation, state setting for nft trasnfer & token transfer * keep validation logic in components, emit isValid flag * check contract interaction params * enforce positive value for sending tokens * remove top level valid flag * handle value validation * set invalid method params for validation * validate raw tx inputs * remove error message for decimal values * pad bytes32 * clean up --------- Co-authored-by: Gerhard Steenkamp <gerhard@umaproject.org>
Karank224 · Feb 13, 2024 · 0244fd4 · 0244fd4
1 parent 1226306
commit 0244fd4
Show file tree

Hide file tree

Showing 16 changed files with 314 additions and 120 deletions.
diff --git a/src/components/Ui/UiInput.vue b/src/components/Ui/UiInput.vue
@@ -13,8 +13,8 @@ const props = defineProps<{
 
 const emit = defineEmits(['update:modelValue', 'blur']);
 
-function handleInput(e) {
-  const input = e.target.value;
+function handleInput(e: Event) {
+  const input = (e.target as HTMLInputElement).value;
   if (props.number) {
     return emit('update:modelValue', !input ? undefined : parseFloat(input));
   }

diff --git a/src/composables/useFormSpaceProposal.ts b/src/composables/useFormSpaceProposal.ts
@@ -2,6 +2,7 @@ import { useStorage } from '@vueuse/core';
 import { clone } from '@snapshot-labs/snapshot.js/src/utils';
 import schemas from '@snapshot-labs/snapshot.js/src/schemas';
 import { validateForm } from '@/helpers/validation';
+import { OsnapPluginData } from '@/plugins/oSnap/types';
 
 interface ProposalForm {
   name: string;
@@ -15,6 +16,7 @@ interface ProposalForm {
   metadata: {
     plugins: {
       safeSnap?: { valid: boolean };
+      oSnap?: OsnapPluginData;
     };
   };
 }

diff --git a/src/plugins/oSnap/Create.vue b/src/plugins/oSnap/Create.vue
@@ -14,10 +14,12 @@ import {
   Transaction
 } from './types';
 import {
+  allTransactionsValid,
   getGnosisSafeBalances,
   getGnosisSafeCollectibles,
   getIsOsnapEnabled,
-  getModuleAddressForTreasury
+  getModuleAddressForTreasury,
+  validateOsnapTransaction
 } from './utils';
 import OsnapMarketingWidget from './components/OsnapMarketingWidget.vue';
 
@@ -51,7 +53,6 @@ function addTransaction(transaction: Transaction) {
 
 function removeTransaction(transactionIndex: number) {
   if (!newPluginData.value.safe) return;
-
   newPluginData.value.safe.transactions.splice(transactionIndex, 1);
   update(newPluginData.value);
 }
@@ -76,7 +77,6 @@ async function fetchBalances(network: Network, safeAddress: string) {
   if (!safeAddress) {
     return [];
   }
-
   try {
     const balances = await getGnosisSafeBalances(network, safeAddress);
 
@@ -214,7 +214,7 @@ watch(newPluginData, async () => {
 onMounted(async () => {
   isLoading.value = true;
   safes.value = await createOsnapEnabledSafes();
-  newPluginData.value.safe = safes.value[0];
+  newPluginData.value.safe = cloneDeep(safes.value[0]);
   tokens.value = await fetchBalances(
     newPluginData.value.safe.network,
     newPluginData.value.safe.safeAddress
@@ -233,7 +233,8 @@ onMounted(async () => {
     <div class="rounded-2xl border p-4 text-md">
       <h2 class="mb-2">Warning: Multiple oSnap enabled plugins detected</h2>
       <p class="mb-2">
-      For best experience using oSnap, please remove the SafeSnap plugin from your space.
+        For best experience using oSnap, please remove the SafeSnap plugin from
+        your space.
       </p>
     </div>
   </template>

diff --git a/src/plugins/oSnap/components/Input/Amount.vue b/src/plugins/oSnap/components/Input/Amount.vue
@@ -1,11 +1,14 @@
 <script setup lang="ts">
 import { formatUnits, parseUnits } from '@ethersproject/units';
+import { amountPositive } from '../../utils';
 
 const props = defineProps<{
   modelValue: string;
   label: string;
   decimals: number | undefined;
+  enforcePositiveValue?: boolean;
 }>();
+
 const emit = defineEmits<{
   'update:modelValue': [value: string];
 }>();
@@ -16,24 +19,36 @@ const dirty = ref(false);
 
 const format = (amount: string) => {
   try {
-    return parseUnits(amount, props.decimals).toString();
-  } catch (error) {
-    return undefined;
+    // empty string throws
+    const parsed = parseUnits(amount, props.decimals).toString();
+    return parsed;
+  } catch {
+    return '0';
   }
 };
 
 const handleInput = () => {
   dirty.value = true;
   const value = format(input.value);
-  isValid.value = !!value;
-  emit('update:modelValue', value ?? '');
+  // empty string value will throw error being converted to BigNumber
+  emit('update:modelValue', value ?? '0');
 };
 
 onMounted(() => {
   if (props.modelValue) {
     input.value = formatUnits(props.modelValue, props.decimals);
   }
 });
+
+watch(input, () => {
+  const value = format(input.value);
+  const valid = !!value;
+  isValid.value = valid;
+  if (props.enforcePositiveValue) {
+    const isPositive = amountPositive(input.value, props.decimals);
+    isValid.value = isPositive;
+  }
+});
 </script>
 
 <template>

diff --git a/src/plugins/oSnap/components/Input/MethodParameter.vue b/src/plugins/oSnap/components/Input/MethodParameter.vue
@@ -3,6 +3,7 @@ import { ParamType } from '@ethersproject/abi';
 import { isAddress } from '@ethersproject/address';
 import { isBigNumberish } from '@ethersproject/bignumber/lib/bignumber';
 import AddressInput from './Address.vue';
+import { hexZeroPad, isBytesLike } from '@ethersproject/bytes';
 
 const props = defineProps<{
   parameter: ParamType;
@@ -17,7 +18,8 @@ const isDirty = ref(false);
 const isBooleanInput = computed(() => props.parameter.baseType === 'bool');
 const isAddressInput = computed(() => props.parameter.baseType === 'address');
 const isNumberInput = computed(() => props.parameter.baseType.includes('int'));
-const isBytesInput = computed(() => props.parameter.baseType.includes('bytes'));
+const isBytesInput = computed(() => props.parameter.baseType === 'bytes');
+const isBytes32Input = computed(() => props.parameter.baseType === 'bytes32');
 const isArrayInput = computed(
   () =>
     props.parameter.baseType === 'array' || props.parameter.baseType === 'tuple'
@@ -28,6 +30,7 @@ const inputType = computed(() => {
   if (isAddressInput.value) return 'address';
   if (isNumberInput.value) return 'number';
   if (isBytesInput.value) return 'bytes';
+  if (isBytes32Input.value) return 'bytes32';
   if (isArrayInput.value) return 'array';
   return 'text';
 });
@@ -40,6 +43,7 @@ const isInputValid = computed(() => {
   if (isAddressInput.value) return isAddress(newValue.value);
   if (isArrayInput.value) return validateArrayInput(newValue.value);
   if (isNumberInput.value) return validateNumberInput(newValue.value);
+  if (isBytes32Input.value) return validateBytes32Input(newValue.value);
   if (isBytesInput.value) return validateBytesInput(newValue.value);
   return true;
 });
@@ -52,17 +56,26 @@ watch(props.parameter, () => {
 });
 
 watch(newValue, () => {
-  if (isInputValid.value) {
-    emit('updateParameterValue', newValue.value);
-  }
+  emit('updateParameterValue', newValue.value);
 });
 
 function validateNumberInput(value: string) {
   return isBigNumberish(value);
 }
 
 function validateBytesInput(value: string) {
-  return value.startsWith('0x');
+  return isBytesLike(value);
+}
+
+function validateBytes32Input(value: string) {
+  try {
+    if (value.slice(2).length > 64) {
+      throw new Error('String too long');
+    }
+    return isBytesLike(value);
+  } catch {
+    return false;
+  }
 }
 
 function validateArrayInput(value: string) {
@@ -84,6 +97,12 @@ function onChange(value: string) {
   newValue.value = value;
   isDirty.value = true;
 }
+
+function formatBytes32() {
+  if (isBytes32Input) {
+    newValue.value = hexZeroPad(newValue.value, 32);
+  }
+}
 </script>
 
 <template>
@@ -130,6 +149,16 @@ function onChange(value: string) {
   >
     <template #label>{{ label }}</template>
   </UiInput>
+  <UiInput
+    v-if="inputType === 'bytes32'"
+    placeholder="0x123abc"
+    :error="!isInputValid && `Invalid ${parameter.baseType}`"
+    :model-value="value"
+    @blur="formatBytes32"
+    @update:model-value="onChange($event)"
+  >
+    <template #label>{{ label }}</template>
+  </UiInput>
   <UiInput
     v-if="inputType === 'text'"
     placeholder="a string of text"

diff --git a/src/plugins/oSnap/components/TransactionBuilder/ContractInteraction.vue b/src/plugins/oSnap/components/TransactionBuilder/ContractInteraction.vue
@@ -1,5 +1,4 @@
 <script setup lang="ts">
-import { parseAmount } from '@/helpers/utils';
 import { FunctionFragment } from '@ethersproject/abi';
 import { isAddress } from '@ethersproject/address';
 
@@ -8,14 +7,15 @@ import {
   createContractInteractionTransaction,
   getABIWriteFunctions,
   getContractABI,
-  validateTransaction
+  parseValueInput
 } from '../../utils';
 import AddressInput from '../Input/Address.vue';
 import MethodParameterInput from '../Input/MethodParameter.vue';
 
 const props = defineProps<{
   network: Network;
   transaction: ContractInteractionTransaction;
+  setTransactionAsInvalid: () => void;
 }>();
 
 const emit = defineEmits<{
@@ -24,7 +24,7 @@ const emit = defineEmits<{
 
 const to = ref(props.transaction.to ?? '');
 const isToValid = computed(() => {
-  return to.value === '' || isAddress(to.value);
+  return to.value !== '' && isAddress(to.value);
 });
 const abi = ref(props.transaction.abi ?? '');
 const isAbiValid = ref(true);
@@ -37,43 +37,46 @@ const selectedMethod = computed(
     methods.value.find(method => method.name === selectedMethodName.value) ??
     methods.value[0]
 );
+
 const parameters = ref<string[]>([]);
 
 function updateTransaction() {
-  if (!isValueValid || !isToValid || !isAbiValid) return;
   try {
+    if (!isToValid.value) {
+      throw new Error('"TO" address invalid');
+    }
+    if (!isAbiValid.value) {
+      throw new Error('ABI invalid');
+    }
+    if (!isValueValid.value) {
+      throw new Error('Value invalid');
+    }
+    // throws is method params are invalid
     const transaction = createContractInteractionTransaction({
       to: to.value,
       value: value.value,
       abi: abi.value,
       method: selectedMethod.value,
       parameters: parameters.value
     });
-
-    if (validateTransaction(transaction)) {
-      emit('updateTransaction', transaction);
-      return;
-    }
-  } catch (error) {
-    console.warn('ContractInteraction - Invalid Transaction:',error);
+    emit('updateTransaction', transaction);
+  } catch {
+    props.setTransactionAsInvalid();
   }
 }
 
 function updateParameter(index: number, value: string) {
   parameters.value[index] = value;
-  updateTransaction();
 }
 
 function updateMethod(methodName: string) {
   parameters.value = [];
   selectedMethodName.value = methodName;
-  updateTransaction();
 }
 
 function updateAbi(newAbi: string) {
   abi.value = newAbi;
   methods.value = [];
-
   try {
     methods.value = getABIWriteFunctions(abi.value);
     isAbiValid.value = true;
@@ -82,27 +85,32 @@ function updateAbi(newAbi: string) {
     isAbiValid.value = false;
     console.warn('error extracting useful methods', error);
   }
-  updateTransaction();
 }
 
 async function updateAddress() {
   const result = await getContractABI(props.network, to.value);
   if (result && result !== abi.value) {
     updateAbi(result);
   }
-  updateTransaction();
 }
 
 function updateValue(newValue: string) {
-  value.value = newValue;
   try {
-    parseAmount(newValue);
+    const parsed = parseValueInput(newValue);
+    value.value = parsed;
     isValueValid.value = true;
   } catch (error) {
     isValueValid.value = false;
+  } finally {
+    updateTransaction();
   }
-  updateTransaction();
 }
+
+watch(to, updateTransaction);
+watch(abi, updateTransaction);
+watch(selectedMethodName, updateTransaction);
+watch(selectedMethod, updateTransaction);
+watch(parameters, updateTransaction, { deep: true });
 </script>
 
 <template>
@@ -137,8 +145,11 @@ function updateValue(newValue: string) {
         </option>
       </UiSelect>
 
-      <div v-if="selectedMethod && selectedMethod.inputs.length">
-        <div class="divider"></div>
+      <div
+        v-if="selectedMethod && selectedMethod.inputs.length"
+        class="flex flex-col gap-2"
+      >
+        <div class="divider h-[1px] bg-skin-border my-3" />
 
         <MethodParameterInput
           v-for="(input, index) in selectedMethod.inputs"