Skip to content

veda-86 #425

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.

Sign up for GitHub

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

Open
kingofclubstroyDev wants to merge 1 commit into
base: master
Choose a base branch
from
Open

veda-86 #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions client/library/library/audits/veda-86.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<page
clientName="Veda Labs"
reportDate="May 29th, 2026"
auditTitle="Veda A-86"
auditVersion="1.0.0"
layout="/library/audits/_layout.html"
repoUrl="https://github.com/Veda-Labs/boring-vault"
repoCommitHash="71368995faf419c75fffa3c687ab27fd58dc2cdb"
finalCommitHash="aac9c31bb0eb35f9274b30ad302cfdbfd6a0443c"
>

<content-for name="schedule">
The security audit was performed by the Macro security team on April 20th to April 27th 2024.
</content-for>

<content-for name="spec">
<ul>
<li>Discussions with the {{page.clientName}} team.</li>
<li>Available documentation in the repository.</li>
</ul>

<template type="audit-markdown">
### Trust Model, Assumptions, and Accepted Risks (TMAAR)

#### Roles

- Strategist: Can swap, as well as submit, cancel and replace orders. Is trusted to use the swapper to facilitate the vaults strategy and operate in the best interest of vault shareholders
- Vault Admin: Has pause control of the swapper and adaptors, can set pricing value like oracles and slippage, approve verified adaptors for the swapper to use and allow token swap routes, and sweep tokens as necessary. Is trusted to set these value appropriately and in the interest of vault shareholders .
- Veda Admin: Has pause control, can set the fee registry on swappers, add or remove verified adaptors on the adaptor registry, and control the fee registry. Is trusted to can in the best interest of their clients and vault shareholders in general.

</template>
</content-for>



<content-for name="source-code">
<p>
Specifically, we audited the following contracts within this repository.
</p>

<template type="file-hashes">
8dd82ac0cd9c40ca05b35dfbb30a5c20dffd7a3428d7d36e9a6cc10a1c7560d5 src/base/Periphery/AdapterRegistry.sol
2cce617492aec2d97dd388566656122ef2dff1046df61bd218dee9ebe13e0798 src/base/Periphery/BoringSwapper.sol
619239d77fdb476beb3f0b46b478be258b587dd78878c63fca54c974f13a0084 src/base/Periphery/FeeRegistry.sol
6cb0957c6d1a520f82ade07b0c67f05143d6a63a19c6d0a4cf9e6703ae400c51 src/base/Periphery/adapters/price/PriceValidator.sol
82162e30acc5c8f968a328ed758d312292b05ee8047757a7d9a05d12179bd965 src/base/DecodersAndSanitizers/Protocols/BoringSwapperDecoderAndSanitizer.sol
90af95577ec17523304e39df87971e6e399dc5ba528322786406274a4b9e28b0 src/base/Periphery/adapters/BaseAdapter.sol
eb67794e8da8738c2220a27b1cc5e48001fa841f7e851cc3be8cf969432324c4 src/base/Periphery/adapters/CowswapAdapter.sol
094a1f8c77a18b937d37f405e279cb8c6d16927b2cb522d59537db69013ec3e7 src/base/Periphery/adapters/LifiAdapter.sol
407588a51c3a8dddec0548e86bbb050cb033fe67e760678bf1cae3f388d610e8 src/base/Periphery/adapters/OneInchAdapter.sol
0b3646af698956cc862c1a788afb0bff8fcc432a1244cccd7d608755c44b497f src/base/Periphery/adapters/OpenOceanAdapter.sol
58cb42b9a2e2f1d8d5038cde01439b816315919966104fc338c34c0491ae01bb src/base/Periphery/adapters/UniswapV3Adapter.sol

</template>

</content-for>
</page>
</content-for>
103 changes: 103 additions & 0 deletions content/collections/public/veda-86-issues.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<item>
<field name="topic">Asset Allocation</field>
<field name="impact">medium</field>
<field name="chance">low</field>
<field name="status">fixed</field>
<field name="commit">ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5</field>
<field name="content">
## [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.
</field>
</item>

<item>
<field name="topic">Fees</field>
<field name="impact">high</field>
<field name="chance">medium</field>
<field name="status">fixed</field>
<field name="commit">ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5</field>
<field name="content">
## [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.
</field>
</item>

<item>
<field name="topic">Account</field>
<field name="impact">medium</field>
<field name="chance">low</field>
<field name="status">fixed</field>
<field name="commit">ef8eae78cd533e8f3ac28d09ba0ce0e8a9bfe9f5</field>
<field name="content">
## [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

</field>
</item>