cancelVouch doesn't update the voucher index of the last vouch of a borrower properly

cancelVouch doesn't update the index of the last vouch in the corresponding mappings when it is moved in the Vouch array of a borrower. It leads to members not being able to cancel their vouch and members potentially cancelling other people vouch.

A staker or a borrower can call cancelVouch: UserManager.sol#L583-L634

function _cancelVouchInternal(address staker, address borrower) internal { Index memory removeVoucherIndex = voucherIndexes[borrower][staker]; if (!removeVoucherIndex.isSet) revert VoucherNotFound(); // Check that the locked amount for this vouch is 0 Vouch memory vouch = vouchers[borrower][removeVoucherIndex.idx]; if (vouch.locked > 0) revert LockedStakeNonZero(); // Remove borrower from vouchers array by moving the last item into the position // of the index being removed and then poping the last item off the array { // Cache the last voucher Vouch memory lastVoucher = vouchers[borrower][vouchers[borrower].length - 1]; // Move the lastVoucher to the index of the voucher we are removing vouchers[borrower][removeVoucherIndex.idx] = lastVoucher; // Pop the last vouch off the end of the vouchers array vouchers[borrower].pop(); // Delete the voucher index for this borrower => staker pair delete voucherIndexes[borrower][staker]; // Update the last vouchers coresponsing Vouchee item uint128 voucheeIdx = voucherIndexes[borrower][lastVoucher.staker].idx; vouchees[staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96(); } // Update the vouchee entry for this borrower => staker pair { Index memory removeVoucheeIndex = voucheeIndexes[borrower][staker]; // Cache the last vouchee Vouchee memory lastVouchee = vouchees[staker][vouchees[staker].length - 1]; // Move the last vouchee to the index of the removed vouchee vouchees[staker][removeVoucheeIndex.idx] = lastVouchee; // Pop the last vouchee off the end of the vouchees array vouchees[staker].pop(); // Delete the vouchee index for this borrower => staker pair delete voucheeIndexes[borrower][staker]; // Update the vouchee indexes to the new vouchee index voucheeIndexes[lastVouchee.borrower][staker].idx = removeVoucheeIndex.idx; } emit LogCancelVouch(staker, borrower); } /** * Cancels a vouch between a staker and a borrower. * @dev The function can only be called by a member of the stakers list. * @param staker The address of the staker who made the vouch. * @param borrower The address of the borrower who received the vouch. */ function cancelVouch(address staker, address borrower) public onlyMember(msg.sender) whenNotPaused { if (staker != msg.sender && borrower != msg.sender) revert AuthFailed(); _cancelVouchInternal(staker, borrower); }

The first part does this right:

Move the lastVoucher to the index of the voucher we are removing in the voucher[borrower] array.

Pop the last vouch off the end of the voucher[borrower] array.

Delete the index in voucherIndexes[borrower][staker].

{ // Cache the last voucher Vouch memory lastVoucher = vouchers[borrower][vouchers[borrower].length - 1]; // Move the lastVoucher to the index of the voucher we are removing vouchers[borrower][removeVoucherIndex.idx] = lastVoucher; // Pop the last vouch off the end of the vouchers array vouchers[borrower].pop(); // Delete the voucher index for this borrower => staker pair delete voucherIndexes[borrower][staker]; // Update the last vouchers coresponsing Vouchee item uint128 voucheeIdx = voucherIndexes[borrower][lastVoucher.staker].idx; vouchees[staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96(); }

Since the last vouch for this borrower has been moved, his voucher index have to be changed. It needs to be updated at two places, in voucherIndexes[borrower][lastStaker.staker] and in vouchees[lastStaker.staker][voucheeIdx].voucherIndex.

But instead, there is a first confusion and voucherIndexes mapping is used to derive a voucheeIdx which is therefore a voucher index and not a vouchee index. Then there is second problem, vouchees[staker][voucheeIdx].voucherIndex is updated instead of vouchees[lastStaker.staker][voucheeIdx].voucherIndex. UserManager.sol#L602-L604

// Update the last vouchers coresponsing Vouchee item uint128 voucheeIdx = voucherIndexes[borrower][lastVoucher.staker].idx; vouchees[staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96();

The last vouch not having his index updated correctly, will lead to two main outcomes:

The associated staker or borrower will be unable to cancel the vouch because the call to cancelVouch will revert if the length of the Vouch array associated to this borrower is not as long anymore. Specifically at this line:

Vouch memory vouch = vouchers[borrower][removeVoucherIndex.idx];

The associated staker or borrower will be able to cancel other members vouch located at the unmodified voucher index if any new stakers's vouch are added to this borrower vouchers mapping.

Staker or borrower of a vouch are unable to cancel a vouch.

Staker or borrower of a vouch can cancel other members vouch. It will impact any function in the contract that iterate on Vouch array of borrowers and therefore will cause to the whole system to malfunction for impacted borrower/staker pairs.

function _cancelVouchInternal(address staker, address borrower) internal { Index memory removeVoucherIndex = voucherIndexes[borrower][staker]; if (!removeVoucherIndex.isSet) revert VoucherNotFound(); // Check that the locked amount for this vouch is 0 Vouch memory vouch = vouchers[borrower][removeVoucherIndex.idx]; if (vouch.locked > 0) revert LockedStakeNonZero(); // Remove borrower from vouchers array by moving the last item into the position // of the index being removed and then poping the last item off the array { // Cache the last voucher Vouch memory lastVoucher = vouchers[borrower][vouchers[borrower].length - 1]; // Move the lastVoucher to the index of the voucher we are removing vouchers[borrower][removeVoucherIndex.idx] = lastVoucher; // Pop the last vouch off the end of the vouchers array vouchers[borrower].pop(); // Delete the voucher index for this borrower => staker pair delete voucherIndexes[borrower][staker]; // Update the last vouchers coresponsing Vouchee item uint128 voucheeIdx = voucherIndexes[borrower][lastVoucher.staker].idx; vouchees[staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96(); } // Update the vouchee entry for this borrower => staker pair { Index memory removeVoucheeIndex = voucheeIndexes[borrower][staker]; // Cache the last vouchee Vouchee memory lastVouchee = vouchees[staker][vouchees[staker].length - 1]; // Move the last vouchee to the index of the removed vouchee vouchees[staker][removeVoucheeIndex.idx] = lastVouchee; // Pop the last vouchee off the end of the vouchees array vouchees[staker].pop(); // Delete the vouchee index for this borrower => staker pair delete voucheeIndexes[borrower][staker]; // Update the vouchee indexes to the new vouchee index voucheeIndexes[lastVouchee.borrower][staker].idx = removeVoucheeIndex.idx; } emit LogCancelVouch(staker, borrower); } /** * Cancels a vouch between a staker and a borrower. * @dev The function can only be called by a member of the stakers list. * @param staker The address of the staker who made the vouch. * @param borrower The address of the borrower who received the vouch. */ function cancelVouch(address staker, address borrower) public onlyMember(msg.sender) whenNotPaused { if (staker != msg.sender && borrower != msg.sender) revert AuthFailed(); _cancelVouchInternal(staker, borrower); }

Manual Review

I recommend updating the code like this:

- uint128 voucheeIdx = voucherIndexes[borrower][lastVoucher.staker].idx; - vouchees[staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96(); + voucherIndexes[borrower][lastStaker.staker] = removeVoucherIndex.idx.toUint96(); + uint128 voucheeIdx = voucheeIndexes[borrower][lastVoucher.staker].idx; + vouchees[lastStaker.staker][voucheeIdx].voucherIndex = removeVoucherIndex.idx.toUint96();