Skip to content
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.

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

sevenSeas-7 #203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 41 additions & 0 deletions client/library/library/audits/sevenSeas-7.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<page
clientName="Seven Seas"
reportDate="May 14, 2024"
auditTitle="Seven Seas A-7"
auditVersion="1.0.0"
repoUrl="https://github.com/Se7en-Seas/boring-vault"
layout="/library/audits/_layout.html"
repoCommitHash="9c6e1e4458fba53f2ff09efe21195dcf570ffc3e"
finalRepoCommitHash="8de7678302b9f52c52315b43e6cd16d9cf6e5121"

>
<content-for name="spec">
The security audit was performed by the Macro security team on May 22nd 2024.
<template type="audit-markdown">
## Trust Model, Assumptions, and Accepted Risks (TMAAR)

Trust Assumptions:

In addition to the trust assumptions mentioned in [SevenSeas-4](https://0xmacro.com/library/audits/sevenSeas-4.html), and [SevenSeas-5](https://0xmacro.com/library/audits/sevenSeas-5.html), this update allows addresses to be blacklisted from interacting with boringVault shares. Users must trust that addresses with the role authorized to update the blacklist will do so in the interest of the protocol and not freeze users assets without good reason.
</template>
</content-for>

<content-for name="source-code">
<p>
Specifically, we audited the following contracts within this repository, and reviewed the migration steps to move assets from the Liquid cellar to a Liquid boring vault.
</p>

<template type="file-hashes">

38253e3f0fb66c856d8293e7ea7b709e7cd05035c208077979c62e7e8d98e24d src/base/BoringVault.sol
c1e9fecdcab6a7896df2f79f1b4b100bb8c9a2849b7a94e673a9279492ae1254 src/base/Roles/TellerWithMultiAssetSupport.sol
154e4924894c1e63f65ca1e30de5138561a77990b48c491b802974c0b84966da src/base/DecodersAndSanitizers/Protocols/EthenaWithdrawDecoderAndSanitizer.sol
5c774f8963326fbfe202cbeba092d2bbed97f263d18283ba574673186773d854 src/migration/CellarMigrationAdaptor.sol
110e76323040580e4e354e8b1f7206acb5c53fa03952022d355e855c0f4a8f56 src/migration/CellarMigrationAdaptor2.sol
cd3021756928dcc1c71819271c7ce077e24c0254b04f2ff55bc1641d22da573d src/migration/CellarMigratorWithSharePriceParity.sol
69d84b13fd18883758c5f54cec4f50aad8e6b555435d814dc64e5b54bd24d5a4 src/migration/ParitySharePriceOracle.sol

</template>

</content-for>
</page>
36 changes: 36 additions & 0 deletions content/collections/public/sevenSeas-7-issues.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<item>
<field name="topic">Use Cases</field>
<field name="impact">medium</field>
<field name="chance">low</field>
<field name="status">fixed</field>
<field name="commit">8de7678302b9f52c52315b43e6cd16d9cf6e5121</field>
<field name="content">
## [L-1] Contracts facilitating transfers may want to be blocked as well

With the addition of a blacklist to boring vaults, currently only the sender and receiver of the shares are passed to the teller in `beforeTransfer()` to check if they are blacklisted:

```solidity
/**
* @notice Implement beforeTransfer hook to check if shares are locked, or if `from` or `to` are on the deny list.
*/
function beforeTransfer(address from, address to) external view {
if (denyList[from] || denyList[to]) revert TellerWithMultiAssetSupport__TransferDenied(from, to);
if (shareUnlockTime[from] >= block.timestamp) revert TellerWithMultiAssetSupport__SharesAreLocked();
}
```
Reference: [TellerWithMultiAssetSupport.sol#L210-L216](https://github.com/Se7en-Seas/boring-vault/blob/9c6e1e4458fba53f2ff09efe21195dcf570ffc3e/src/base/Roles/TellerWithMultiAssetSupport.sol#L210-L216)

However, when shares are transferred via `transferFrom()`, an approved operator facilitates the transfer, but in doing so this could be a contract that is involved in illegal or malicious behaviour that may want to be blocked. This may be unlikely but if contracts can be sanctioned as tornado cash was, in order to remain compliant with current or future regulations, it may be wise to allow for the ability to block operators in case it becomes necessary.
</field>
</item>

<item>
<field name="topic">Informational</field>
<field name="impact">medium</field>
<field name="content">
## [I-1] Addresses can be blacklisted from interacting with boringVault shares

There is now the ability for an authorized role to update a `denyList` which prevents any address on the list from sending, receiving, or facilitating the transfer of vault shares. Although this is an accepted practice, and widely used tokens like USDC also have blacklist functionality, users should be aware that they can have their shares frozen. Blacklisting addresses should typically only be done to freeze assets of malicious users or prevent interaction with undesired contracts. Developers should also be aware that for contracts interacting with these shares, that either users or the contract itself can be blacklisted, which may adversely effect the contracts functionality.
</field>
</item>