# Spearbit Seal 🦭
## Seal Engagements
As a Junior Security Researcher, it can be intimidating to enter the field and keep pace with the rapidly evolving landscape of blockchain technology. The world of Ethereum, and particularly Ethereum security, is constantly changing.
In the past few years, several initiatives have been aimed at helping both projects stay current with the latest trends in Ethereum security, and security researchers gain hands-on experience with real-world audits. Examples of these initiatives include open audits that are accessible to more security researchers, such as *[Code4rena](https://code4rena.com/)* and *[Sherlock](https://www.sherlock.xyz/)* audits, the [*Secureum*](https://secureum.xyz/) epoch ∞ (alongside *RACE-X* quizzes and *CARE-X* events), and bug bounty platforms like *[Immunefi](https://immunefi.com/)*.
**How does Spearbit try to solve this today?**
**SEAL** (*Security Education and Assessment Lab*) engagements started off as Spearbit's approach to upskilling *Junior Security Researchers* (*referred to as JSRs*) and Associate Security Researchers (*ASRs*). **SEAL** is a collaboration between ***Spearbit*** and ***Secureum***. The idea for this was inspired by a shared goal of giving reseachers a tactiful track to gain real-time audit experience alongside team members ranging in expertise. These engagements are led by experienced *Lead Security Researchers (LSRs)* who use a collaborative approach to guide and test the skills of JSRs and ASRs through challenging and comprehensive audits. These engagements aim to help JSRs grow their expertise and advance their careers in the security industry.
The below diagram shows how the initial idea for SEAL engagements tie into allowing upskilling through the [promotion process](https://github.com/spearbit/proposals/discussions/3) within Spearbit.
Participating in guided audits and collaborating with more experienced auditors and researchers can provide numerous benefits for less experienced professionals. One significant benefit is the opportunity to learn from the researchers you’re collaborating with. By working on projects together, JSRs can gain exposure to new protocols, techniques, and an audit mindset they may not have encountered otherwise. Additionally, they can stay informed of the latest protocols in the Ethereum ecosystem, new security vulnerabilities, and attack vectors.
Spearbit has onboarded many Security Researchers from a wide range of backgrounds. Spearbit's team includes individuals from Web2 security roles, as well as those with backgrounds in software development/engineering (both *Web2* and *Web3*), academia and the fincancial sector. Security Researchers from diverse backgrounds bring different skills and perspectives to a team, which is incredibly valuable for enhancing the security of Ethereum-based systems. This can provide a balanced, thorough approach to Web3 security reviews. By utilizing researchers' unique strengths and expertise, teams can more effectively identify potential vulnerabilities and security concerns and propose effective mitigations.
In addition to learning from others, collaborative efforts also provide valuable opportunities for JSRs to practice their skills and gain experience. Spearbit [has audited](https://github.com/spearbit/portfolio/) some of the most significant projects in the Ethereum ecosystem. Working on real-world projects with an extensive team allows JSRs to apply their knowledge and abilities in a practical setting, giving them a sense of what it's like to work in the field. At Spearbit, team size varies depending on the audit and often consists of up to *five or six* people. On the other hand, on *SEAL* audits, the team consists of *at least nine* security reasearchers. By working in a big group, JSRs can learn from different perspectives and approaches, helping them develop a well-rounded understanding of smart contract security.
Another difference between a typical *Spearbit* audit and a *SEAL engagement* is how the potential security issue is communicated within the team. In a Spearbit security review, auditors *typically* open GitHub issues directly when they identify vulnerabilities or potential security concerns. However, during *SEAL* engagements, which also serve as learning experiences for JSRs, issues are kept from being opened directly on GitHub. Instead, an extensive discussion on Discord, often in the form of questions and answers, is utilized. This approach gives Junior Security Researchers an initial push on a specific code snippet to identify vulnerabilities. During the discussion, they can ask questions and express their concerns, and more experienced team members can provide guidance and share their knowledge, experiences, and methodology. This question-and-answer approach helps Junior Researchers identify specific vulnerabilities or areas of concern in the codebase rather than feeling overwhelmed by the entire project. Lead Security Researchers serve as mentors to JSRs, providing guidance and feedback throughout the audit process. They help clarify complex concepts and answer questions. Teamwork offers numerous benefits for JSRs, and the question-and-answer approach can be constructive.
**What's the advantage of having a SEAL security review?**
A *SEAL* engagement can offer many advantages for the project team as well. The team comprises a larger group of researchers who may have less experience but are highly motivated to learn and improve. This approach is more cost-effective than a formal security review and promotes a collaborative environment where JSRs can learn and grow. Additionally, the project team can be more engaged in the process, gaining a deeper understanding of the security aspects of their project. With a larger group, more ground can be covered, and more issues can be identified.
**So what have we learned about SEAL so far?**
As Spearbit continues to iterate and evolve, there is an open thought to exploring further alternatives to the current SEAL model. Although there is value to be found in these kinds of engagements, qualitative evaluations do not completely address the main intention of this model: assessing participants' knowledge and upskilling.
Let us consider the **pros** and **cons** of such a model.
Pros:
* Security Review at a discount for the client.
* Providing more Junior Security Researchers access to an engagement.
* Spearbit community Upskilling.
* Finding and identifying hidden talent which can be utilized for complex engagements.
* Instructing LSRs to become mentors.
* Instructing JSRs to become mentors as they upskill.
Cons:
* Heavy load on LSRs as they need to mentor and assure the review’s quality.
* Low number of LSRs are interested in mentorships (assumption).
* Difficult to evaluate a large number of participants at once.
* Subjective evaluations are tough to commodify because they are not based on data.
* It does not produce objective, actionable data.
In conclusion to the above-mentioned points, this model does lower the cost of certain engagements and provides more opportunities for JSRs to participate. Nevertheless, it does not seem to efficiently address objective evaluations for the promotion process nor produce objective actionable data. Therefore, SEALs should be maintained as an opportunity for educational and upskilling rather than be used primarily for evaluation.
**The future outlook on SEAL**
An alternative to the current SEAL model used for evaluating community member skills for the purpose of promotion is SEAL-X, which takes an individual approach where participants must find the highest number of issues during a limited period of time. The top percentage of participants who find most issues or the highest severity ones will be promoted.
This new model may follow the competitive contest rules but not necessarily its mechanics. As there are two approaches that could be taken:
* Unpaid and running in parallel to another security review.
* It can be a paid contest model where participants get paid and promoted.
To incentivize LSRs and evaluators to take the time to verify participants' findings as well as to prevent participants from reiteratively applying for promotion, the SEAL-X model can have an application fee, which will 100% go to the evaluators. The fee shall not be too high nor too low. Currently, a Junior Security Researcher makes $3,000 a week, so the fee must be low but significant enough to make the applicant think if it is worth the effort. Only those confident in their skills and able to prove themselves will get the promotion.
**Closing Thoughts**
Entering the field of blockchain security can be intimidating, but initiatives such as open audits, bug bounty platforms, and SEAL engagements are making it easier for junior security researchers to gain hands-on experience with real-world audits. Spearbit's SEAL engagements provide numerous benefits for less experienced professionals, including opportunities to learn from experienced researchers, practice their skills, and gain experience. By utilizing researchers' unique strengths and expertise, teams can more effectively identify potential vulnerabilities and security concerns and propose effective mitigations. While there are pros and cons to the current SEAL model, the benefits of this approach are significant, and the future outlook for this model is promising as Spearbit continues to iterate and evolve.
# The contents below will not be published until Paladin audit is public.
## Paladin SEAL experience and findings
The Paladin SEAL engagement was the second SEAL engagement. In this engagement, our team of Junior Security Researchers, under the guidance of [Rajeev](https://twitter.com/0xRajeev) and [Patrick](https://twitter.com/patrickd_de), had the opportunity to audit the Shrine & Sanctum project that was created by the [Paladin team](https://paladin.vote/). Through collaboration, we identified 36 issues ranging from *Informational* to *High* Severity.
In the next section, we will dive deeper into the specific security findings we discovered during the engagement and the steps taken to find them. This experience was beneficial for the project team and our development as Junior Security Researchers. It allowed us to gain valuable hands-on experience and improve our skills in a real-world setting while guided by Rajeev and Patrick.
(Taken from: [https://github.com/PaladinFinance/Shrine/blob/main/Audit Context.md](https://github.com/PaladinFinance/Shrine/blob/main/Audit%20Context.md))
*Shrine & Sanctum* are two systems based on the same primitive: leverage the fact that Convex controls almost all the Curve Gauges total supply, without having enough `veCRV` to max boost their revenue on the Gauges, to farm alongside the Convex pools (with a capped percentage of the Gauge total Supply: *currently 10%*), and purchase boosting power from the Warden system to farm at the maximum multiplier, while increasing the multiplier for the Convex pool for the same Gauge (since they will control a bit less of the total supply).
This system is based on the Curve Gauge logic where an address controlling 25% of a Gauge total supply needs *25%* of all veCRV to max boost the CRV incentives they receive. By using that logic & the boost delegation system introduced with *veBoost*, it allows a system not owning veCRV (and not having to be preoccupied by voting rights, or the peg of the wrapper) to farm alongside the protocols centralizing Curve LPs (mainly Convex), by renting the veBoost from other veCRV holders currently not using their boosting power (via the Warden markets).
### 1. High Severity - Deposits can be forced to revert and cause DoS to gauges
The owner can add a token and its associated gauge to the Shrine contract with a call to the the [`addToken()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L795) function:
```solidity
function addToken(address token, address gauge) external onlyOwner {
. . .
// Check that the given Gauge is listed in the Curve Gauge Controller
if(IGaugeController(GAUGE_CONTROLLER).gauge_types(gauge) < 0) revert Errors.InvalidGauge();
// And that the given token is the one associated with the Gauge
if(IGauge(gauge).lp_token() != token) revert Errors.InvalidToken();
// Whitelist the given token, and update storage with the new token<>gauge couple
. . .
emit TokenAdded(token, gauge);
}
```
Users that want to interact with the protocol and deposit some tokens to the gauge have to call the [`deposit()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L303) function. They have to specify the token they want to deposit and its corresponding amount.
Curve’s`LiquidityGauge` measures liquidity for CRV distribution, stakes LP tokens into an SNX staking rewards contract, and distributes the additional rewards token.
The flow in the [`deposit()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L303) function starts by first claiming the CRV and the extra rewards from the Gauge that matches the given LP token. After that, the LP tokens are pulled from the user, the storage is updated and the tokens are deposited into the Gauge.
```solidity
function deposit(address token, uint256 amount) external nonReentrant whenNotPaused {
. . .
// Claim CRV rewards & any existing extra rewards from the Gauge
_claimCRV(token);
_claimExtraRewards(token);
// Update the user indexes (for CRV & for extra rewards if there is)
_userIndexUpdate(depositor, token);
. . .
// Pull tokens
IERC20(token).safeTransferFrom(depositor, address(this), amount);
// Update storage with user deposit
. . .
// Deposit the tokens into the Gauge
. . .
emit Deposit(depositor, token, amount);
}
```
We will take a deeper look in the `_claimCRV` and in the `_claimExtraRewards` functions.
```solidity
/**
* @dev Claims CRV rewards for a given LP token (from its associated Gauge) & update global reward state
* @param token Address of the LP token
*/
function _claimCRV(address token) internal {
if(lastUpdate[token] >= block.timestamp) return;
// Fetch start balance to track received amount
uint256 prevBalance = IERC20(CRV).balanceOf(address(this));
// Get the CRV rewards from Curve minter for the Gauge
address gauge = tokenToGauge[token];
ICurveMinter(MINTER).mint(gauge);
// Calculate received amount based on start balance
uint256 currentBalance = IERC20(CRV).balanceOf(address(this));
uint256 claimedRewards = currentBalance - prevBalance;
lastUpdate[token] = block.timestamp;
if(claimedRewards > 0 && totalDeposited[token] != 0){
// calculate share of received amount that goes to Reserve & to BoostFees (based set ratios)
// And set the rest as user share, and update the index to reflect the accrued amount
uint256 boostFeeShare = (claimedRewards * boostFeeRatio) / MAX_PCT;
uint256 reserveShare = (claimedRewards * managementFeeRatio) / MAX_PCT;
uint256 userRewardsShare = claimedRewards - (boostFeeShare + reserveShare);
boostFeeAmount += boostFeeShare;
reserveAmount += reserveShare;
rewardIndex[token] += (userRewardsShare * UNIT) / totalDeposited[token];
}
}
```
The goal of the `_claimCRV` function is to claim the CRV rewards for a given LP token from its associated Gauge and to update the global reward state. If the balance of the CRV reward changes, (meaning that the difference of the `currentBalance` and the `prevBalance` is `>0` ) and the total amount of the LP token that is deposited in the Shrine is different than `0` the protocol updates the `boostFeeAmount`,the `reserveAmount` and the `rewardIndex` mappings accordingly.
```solidity
/**
* @dev Claims all extra rewards for a given LP token (from its associated Gauge) & update global reward states
* @param token Address of the LP token
*/
function _claimExtraRewards(address token) internal {
address gauge = tokenToGauge[token];
// Get the count of extra rewards from the Gauge
// If there is no rewards, no need to continue further
uint256 rewardCount = IGauge(gauge).reward_count();
if(rewardCount == 0) return;
// For each extra reward token, get the start balance
uint256[] memory currentBalances = new uint256[](rewardCount);
for(uint256 i; i < rewardCount; i++){
address rewardToken = IGauge(gauge).reward_tokens(i);
currentBalances[i] = IERC20(rewardToken).balanceOf(address(this));
}
// Claim all the rewards from the Gauge
IGauge(gauge).claim_rewards();
for(uint256 i; i < rewardCount; i++){
// And for each reward token, calculated received amount (based on start balance)
address rewardToken = IGauge(gauge).reward_tokens(i);
uint256 newBalance = IERC20(rewardToken).balanceOf(address(this));
uint256 accruedAmount = newBalance - currentBalances[i];
if(accruedAmount > 0){
// If the reward token is new, add it to the local list for that Gauge
if(rewardsPerGauge[gauge][rewardToken].index == 0){
gaugeExtraRewards[gauge].push(rewardToken);
}
// If the received amount is not null, update the index to reflect the accrued amount
rewardsPerGauge[gauge][rewardToken].index += safe128((accruedAmount * UNIT) / totalDeposited[token]);
}
}
}
```
A similar approach is followed in the `_claimExtraRewards` function as well. We can see that for every extra reward the difference of the `startBalance` and the `newBalance` is calculated. If the difference is positive, the state variables for each extra-reward are updated as well.
```solidity
/**
* @dev Claims all extra rewards for a given LP token (from its associated Gauge) & update global reward states
* @param token Address of the LP token
*/
function _claimExtraRewards(address token) internal {
address gauge = tokenToGauge[token];
// Get the count of extra rewards from the Gauge
// If there is no rewards, no need to continue further
uint256 rewardCount = IGauge(gauge).reward_count();
if(rewardCount == 0) return;
// For each extra reward token, get the start balance
uint256[] memory currentBalances = new uint256[](rewardCount);
for(uint256 i; i < rewardCount; i++){
address rewardToken = IGauge(gauge).reward_tokens(i);
currentBalances[i] = IERC20(rewardToken).balanceOf(address(this));
}
// Claim all the rewards from the Gauge
IGauge(gauge).claim_rewards();
for(uint256 i; i < rewardCount; i++){
// And for each reward token, calculated received amount (based on start balance)
address rewardToken = IGauge(gauge).reward_tokens(i);
uint256 newBalance = IERC20(rewardToken).balanceOf(address(this));
uint256 accruedAmount = newBalance - currentBalances[i];
if(accruedAmount > 0){
// If the reward token is new, add it to the local list for that Gauge
if(rewardsPerGauge[gauge][rewardToken].index == 0){
gaugeExtraRewards[gauge].push(rewardToken);
}
// If the received amount is not null, update the index to reflect the accrued amount
rewardsPerGauge[gauge][rewardToken].index += safe128((accruedAmount * UNIT) / totalDeposited[token]);
}
}
}
```
However, a zero-address check for the `totalDeposited[token]` is missing here, in the line [`L576`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L576`). The zero-address check is present in the similar [`_claimCRV`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L519) function, in the line [`L535`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L535).
If the value of the `totalDeposited`mapping is `0` while the `accruedAmount` is `>0` the execution will revert due to a division-by-zero. But can we force this to happen while the `accruedAmount` is still `>0` ?🤔
From the [Curve DAO docs](https://curve.readthedocs.io/dao-gauges.html):
`LiquidityGaugeV2`
The v2 liquidity gauge adds a *full ERC20 interface* to the gauge, tokenizing deposits so they can be directly transferred between accounts without having to withdraw and redeposit. It also improves flexibility for onward staking, enabling or disabled at any time and handling up to eight reward tokens at once.
As newer versions of the Curve gauge have a full ERC20 interface, it’s possible! An attacker has to transfer some funds to the Shrine contract by calling the `.transfer` function of the gauge contract `(IGauge(gauge).transfer(address(shrine), 3500);)`, which is present in the newer versions of the Gauge. After the funds are transferred, the Shrine contract will start earning some extra rewards!
So the exploit-scenario looks like this:
1. The attacker targets Shrine protocol and deposits some funds to the gauges(`IGauge(gauge).deposit(5_000);`)which have some extra rewards enabled i.e. `rewardCount(IGauge(gauge).reward_count();) > 0`.
2. The attacker transfers those funds to the Shrine contract by calling the `.transfer` function on the gauge contract (`IGauge(gauge).transfer(address(shrine), 3500);`), which is present in the newer(`≥2`) versions of the gauge.
3. After some time, Shrine earns extra rewards because it has some staked LP tokens from the attacker’s transfer.
4. Shrine’s owner calls the [`addToken()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L795) function on the gauge that was targeted by the attacker.
5. When users try to deposit funds into Shrine by calling the[`deposit()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L303) function, the deposits will fail as the internal`_claimExtraRewards` reverts due to a division-by-zero for `totalDeposited[token]` which is still `0` as per the protocol’s accounting (`rewardsPerGauge[gauge][rewardToken].index += safe128((accruedAmount * UNIT) /totalDeposited[token]);`).
6. The attacker has effectively blocked gauges with `reward_count() > 0` in the Shrine protocol. The attacker’s action can be triggered before or after the addition of gauges by the owner, but before the first deposit.
The attacker has effectively caused a DoS to the gauges deposits who have the extra-rewards functionality enabled and prevented the protocol's bootstrapping!
### 2. Medium Severity - A token with an unsupported gauge can be added to Shrine
As previously mentioned, not all versions of the `LiquidityGauge` contract implement the same interface. The Shrine, when interacting with the gauge, uses some functions that aren’t available in all Gauge versions.
The Shrine uses the [`reward_count()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L558) function to count the *available rewards* for a given Gauge and the [`reward_tokens()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L564) function to *fetch the addresses of the reward tokens*.
During the engagement we have identified that's possible for a token to be accidentally added with an unsupported gauge version as the [`addToken()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L795) function, as this function does not check the gauge version or if the gauge implements the functions used by the Shrine.
Adding an unsupported gauge may cause problems and confusion among users about which gauges are functional and which are not. Calling the [`deposit()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L303) function with the unsupported gauge will fail as the internal function [`_claimExtraRewards()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L553) will revert.
Furthermore, it's also possible that more functions of the Shrine contract, such as the [`updateState()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L493), the [`claim()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L383), and the [`claimAllExtraRewards()`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L450) function will also revert if an unsupported gauge version is added.
**Response from the Paladin Team:**
This issue was considered while implementing the `_claimExtraRewards()`, but turns out to be ignored by the other pre-requisite for adding a Gauge to `Shrine/Sanctum`: the Gauge needs to use the `VotingEscrowBoost` logic (otherwise the rented boost will not affect the yield farmed on that Gauge), which excludes the oldest Gauges from being usable in `Shrine/Sanctum`.
The Gauges that will be accepted for the whitelist are the Gauges that have the following:
Which are the following Gauge versions:
• **[LiquidityGaugeV4](https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/gauges/LiquidityGaugeV4.vy)**
• **[LiquidityGaugeV5](https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/gauges/LiquidityGaugeV5.vy)**
• **[Factory LiquidityGauge](https://github.com/curvefi/curve-factory/blob/master/contracts/LiquidityGauge.vy)**
And all those versions have `reward_count()` & `IGauge(gauge).reward_tokens(i)` , allowing the method to execute correctly.
### 3. Medium Severity - Deviation from EIP4626 standard can affect composability
As we looked on the [`SanctumVault`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L23) contract one of the first things that crossed our minds is whether the [`SanctumVault`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L23) contract is fully compatible with the [`EIP4626`](https://eips.ethereum.org/EIPS/eip-4626) standard.
The [`EIP4626`](https://eips.ethereum.org/EIPS/eip-4626) standard allows for the implementation of a standard API for tokenized Vaults representing shares of a single underlying [EIP-20](https://eips.ethereum.org/EIPS/eip-20) token. This standard is an extension on the [`EIP-20`](https://eips.ethereum.org/EIPS/eip-20)` token that provides basic functionality for depositing and withdrawing tokens and reading balances.
In the **[`maxDeposit`](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4626.md#maxdeposit)** and in the [`maxMint`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L232) methods it’s noted that:
>
>
>
> **MUST** return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which **MUST NOT** be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. **MUST NOT** *rely on balanceOf of asset*.
>
The [`maxMint`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L232) function:
```solidity
function maxMint(address owner) public view virtual returns (uint256) {
uint256 userAssetBalance = IERC20(token).balanceOf(owner);
uint256 availableDeposit = ISanctumStrategy(strategy).availableDeposit(gauge, userAssetBalance);
// In case the user has a bigger balance then the remaining available amount in the Strategy, send the user balance
// Otherwise, send the remaining available amount (shares & assets are 1:1)
return userAssetBalance > availableDeposit ? availableDeposit : userAssetBalance;
}
```
The [`maxDeposit`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L217) function:
```solidity
function maxDeposit(address owner) public view virtual returns (uint256) {
uint256 userAssetBalance = IERC20(token).balanceOf(owner);
uint256 availableDeposit = ISanctumStrategy(strategy).availableDeposit(gauge, userAssetBalance);
// In case the user has a bigger balance then the remaining available amount in the Strategy, send the user balance
// Otherwise, send the remaining available amount (shares & assets are 1:1)
return userAssetBalance > availableDeposit ? availableDeposit : userAssetBalance;
}
```
However, as we can see the [`maxMint`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L232) and the [`maxDeposit`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/sanctum/SanctumVault.sol#L217) functions of the `SanctumVault` contract *does* rely on the balance of the user, as it uses it to estimate the future `totalSupply` of the Gauge
***Note*:** [A great introduction to `EIP-4626`](https://www.youtube.com/watch?v=L8dijE5qhTg) is given by ****[Joey Santoro](https://twitter.com/joey__santoro?lang=el)**** in the **[Solidity Fridays live stream series](https://twitter.com/SolidityFridays)**
### 4. Medium Severity - ShrineAdept logic/cap can be manipulated
**From the Shrine documentation**
> A `ShrineAdpet` contract is deployed to act as a proxy between a *DAO/Protocol* and *Shrine*, and to handle the deposits in both *Shrine & Convex* at the same time. This contract will prioritize the deposit into Shrine, up to the deposit cap, and will then deposit into Convex. And the contract will first withdraw from Convex before withdrawing from Shrine.
>
The [deposit](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L141-L165) function:
```solidity
function deposit(uint256 amount) external onlyDepositor {
if(amount == 0) revert Errors.NullAmount();
// Deposit in priority into Shrine to go up to the deposit cap
// Then deposit the rest in Convex
// (calculations of the split to reach Shrine deposit cap take in account
//the new amount being deposited by this method)
uint256 shrineAvailableDeposit = shrine.availableDeposit(token, amount);
uint256 shrineAmount = shrineAvailableDeposit > amount ? amount : shrineAvailableDeposit;
uint256 reaminingAmount = amount - shrineAmount;
// Pull tokens
IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
// Need to do the Convex deposit before to increase Gauge total
//deposited before the Shrine deposit
// (if the amount to deposit is not null)
if(reaminingAmount > 0){
IConvexBooster(CONVEX_BOOSTER).deposit(pid, reaminingAmount, true);
}
//Then deposit into Shrine to reach the deposit cap
if(shrineAmount > 0){
shrine.deposit(token, shrineAmount);
}
}
```
The `shrineAvailableDeposit` is calculated as: `shrine.availableDeposit(token, amount);`
This logic can be manipulated by sandwiching [`ShrineAdept`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L21) deposits in-between depositing and withdrawing directly to/from gauge to inflate its total supply temporarily such that deposits pass the *availableDeposit* check for the *10%* deposit cap on the yet-to-be-deposited added supply and making the deposit go to Shrine instead of Convex. For example, the motive for reserve/boost managers to do so will be to get more fee share from CRV rewards. The reverse can also be done to direct deposits to Convex instead of Shrine.
Suppose that an attacker wants to block the future deposits into the Shrine contract - by forcing the Shrine contract to hit its supply cap. The attacker has to:
- Watch the public mempool for a transaction that calls the [deposit](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L141-L165) function of the [`ShrineAdept`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L21) contract. This transaction has to transfer a lot of tokens in the [`ShrineAdept`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L21) contract.
- Frontrun the call to the [deposit](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L141-L165) function by a first call that removes funds from either the `Convex` or the Curve itself. This will redirect more funds to the Shrine contract as the `shrineAvailableDeposit = shrine.availableDeposit(token, amount);` depends on the `totalSupply` of the Gauge contract - which is now reduced.
- After the user’s transaction is executed, the attacker has to redeposit the funds back to the `Convex/Gauge` contracts.
- After the attacker’s action a direct call to the Shrine contract will fail as the [`availableDeposit`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/Shrine.sol#L273-L278) will be `0`. If the user called the [`ShrineAdept`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L21) contract instead, the whole amount would go to Convex as the [`shrineAvailableDeposit`](https://github.com/PaladinFinance/Shrine/blob/804617bf24c0847373c66555132f96198c676119/contracts/shrine/ShrineAdept.sol#L148) will be `0`.
This allows Shrine/Convex users to abuse the deposit cap logic and direct deposits to their preferred protocol by sandwiching and temporarily manipulating protocol liquidity with the goal of capturing more fees or to simply grief.
**Response from the Paladin Team:**
The missing [`rebalance()`](https://github.com/PaladinFinance/Shrine/blob/274da4e2c5cfb5229e0f657e0029096792f61c74/contracts/shrine/ShrineAdept.sol#L265) method was added [`PaladinFinance/Shrine#1`,](https://github.com/PaladinFinance/Shrine/pull/1) allowing to
rebalance the deposits between **Shrine & Convex**. If deposits are impacted by the sandwich attack, it will affect the yield of the depositor (either reduce the one on Shrine because the deposited amount cannot be fully covered by the rented boost, or dilute the Convex yield more than expected). So such manipulations from the depositor can be seen as inefficient (and if they want to reduce the share deposited into Shrine, they are free to deposit directly themselves the exact amount, instead of using the **ShrineAdept**). And with the `rebalance()` system, that would need to consistently sandwich such transaction, which can be an inefficient strategy,
**References:**
- [Solidity Fridays with Joey Santoro | ERC 4626 Discussion](https://www.youtube.com/watch?v=L8dijE5qhTg)
-
The full report can be found here:
Sign in with Wallet
Connect another wallet