diff --git a/client/library/library/audits/veda-86.html b/client/library/library/audits/veda-86.html new file mode 100644 index 00000000..e57e06f1 --- /dev/null +++ b/client/library/library/audits/veda-86.html @@ -0,0 +1,58 @@ + + + + The security audit was performed by the Macro security team on April 20th to April 27th 2024. + + + + + + + + + + + +

+ Specifically, we audited the following contracts within this repository. +

+ + + + + + diff --git a/content/collections/public/veda-86-issues.html b/content/collections/public/veda-86-issues.html new file mode 100644 index 00000000..89b25869 --- /dev/null +++ b/content/collections/public/veda-86-issues.html @@ -0,0 +1,103 @@ + + Asset Allocation + medium + low + fixed + ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5 + + ## [H-1] Limit order collateral can be swept to different receiver + + [`BoringSwapper`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol) facilitates both limit orders and instant swapping with registered protocols. In the case of limit orders, assets are pulled and stored in the swapper contract until they are later filled. However, there are two cases where excess balance of a token, that is not locked as unclaimed fees, is considered dust and swept out of the contract as “dust”. This can occur after a [`swap()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L162-L174) sending to the dust to the swap recipient: + + ```solidity + // Return any unspent tokenIn dust to the vault, excluding locked limit order funds. + // @dev `locked` only accounts for fee buckets, not pending order inputAmounts. If multiple + // concurrent instant swaps for the same tokenIn are in flight, one could sweep the other's + // unspent input as dust. Acceptable given Auth-gated callers and single-swap usage patterns. + uint256 locked = feesInToken[swapConfig.tokenRoute.tokenIn] + claimableFees[swapConfig.tokenRoute.tokenIn]; + uint256 balance = swapConfig.tokenRoute.tokenIn.balanceOf(address(this)); + uint256 dust = balance > locked ? balance - locked : 0; + if (dust > 0) swapConfig.tokenRoute.tokenIn.safeTransfer(address(swapConfig.receiver), dust); + ``` + Reference: [BoringSwapper.sol#L515-522](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L515-L522) + + As well as to any recipient via `sweep():` + + ```solidity + /// @notice Reclaims any token sitting on the swapper back to a vault. + /// @dev claimable by swapper admin + function sweep(ERC20 token, BoringVault vault) external requiresAuth { + uint256 locked = feesInToken[token] + claimableFees[token]; + uint256 balance = token.balanceOf(address(this)); + uint256 sweepable = balance > locked ? balance - locked : 0; + if (sweepable > 0) token.safeTransfer(address(vault), sweepable); + + emit Swept(token, address(vault), sweepable); + } + ``` + Reference: [BoringSwapper.sol#L387-396](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L387-L396) + + In the case where there is pending limit order collateral of the same token type that is being swapped or swept, it is considered excess and send to the swap receiver or set sweep receiver, while leaving the swaps fee in the contract. Although `sweep()` is admin controlled and trusted, and the swap recipient is validated via merkle root prior to being called by the vault, there is no assurance that these addresses will be the same as the vault that submitted the limit order, which would result in a loss of funds for the vault and an unexpected amount of tokens being sent to the recipient. + + **Remediations to Consider** + + Either prevent both swaps and sweeps from occurring while a limit order using the same tokenIn is pending, clearing it up on an order’s cancelation or on [`releaseFee()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L398-L414) once filled. + + Or keep track of pending limit orders and include them in the locked balance calculations to prevent in being included as dust when sweeping. + + + + + Fees + high + medium + fixed + ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5 + + ## [H-2] Limit order fees can be skipped + + When a limit order is made, assets are held in collateral along with fees to be taken. These fees are book kept in `feesInToken`, and are either refunded if the order is canceled or claimed by a privileged address trusted to call [`releaseFee()`](feesInToken) once the order has been filled. However, there is a period of time where the order has been filled and [`releaseFee()`](feesInToken) has not been called that a strategist could frontrun and call [`cancelOrder()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L189-L192) to be refunded their fees before they are claimed by the protocol. + + **Remediations to Consider** + + Rather than immediately refund the fee, track each refund with a timestamp and only allow claiming after a set duration has elapsed. Then add a privileged function that can invalidate a fee refund. + + + + + Account + medium + low + fixed + ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5 + + ## [M-1] Swept limit order collateral can lead to inaccurate fee bookkeeping + + As mentioned in H-1, limit order collateral can be swept by [`swap()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L162-L174) or [`sweep()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L387-L396), these swept assets do not move the assets tracked by fees in `feesInToken` or `claimableFees` which is returned while canceled: + + ```solidity + uint256 balance = record.tokenIn.balanceOf(address(this)); + uint256 refund = balance < record.inputAmount + record.fee ? balance : record.inputAmount + record.fee; + if (refund > 0) { + record.tokenIn.safeTransfer(address(record.receiver), refund); + } + // Only deduct the portion of the fee still physically in the contract — guards against + // underflow when a partial fill consumed some or all of the non-fee tokens. + uint256 feeDeducted = record.fee < balance ? record.fee : balance; + feesInToken[record.tokenIn] -= feeDeducted; + ``` + Reference: [BoringSwapper.sol#L234-242](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L234-L242) + + Even in the case of the limit order being swept, the fees should still be present in the contract and properly refunded to the vault, and properly setting feesInToken to zero. + + However, if another limit order is made with the same `tokenIn` before the swept limit order is canceled, then there will be more assets available to refund the swept order. If this new order is smaller than the canceled swept order, then all the balance would go into refunding it, leaving the contract with potentially no balance but it would still hold the fees in feesInToken for the new order yet not have the balance to cover it. + + Additionally this can also occur if there is an active limit order, and [`cancelOrder()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L189-L192) is called after the order is filled but before [`releaseFee()`](https://github.com/Veda-Labs/boring-vault/blob/71368995faf419c75fffa3c687ab27fd58dc2cdb/src/base/Periphery/BoringSwapper.sol#L398-L414) is called on the order. + + **Remediations to Consider** + + Same remedies as H-1, book keep pending limit order collateral, or prevent swaps, sweeps, and limit orders of the same `tokenIn` while there is a pending order + + + +