# maxgrok x Crab Protocol
Audit Firm: maxgrok
Client Firm: Crab Protocol
Prepared By: maxgrok
Delivery Date: December 28th, 2023
<br />
Crab Protocol engaged maxgrok to review the security of its Smart Contract system. From December 4th, 2023 to December 28th, 2023, a team of 1 auditor reviewed the source code in scope. All findings have been recorded in the following report.
Notice that the examined smart contracts are not resistant to external/internal exploit. For a detailed understanding of risk severity, source code vulnerability, and potential attack vectors, refer to the complete audit report below.
# Project Overview
| Project Name | Crab Protocol |
|--------------|----------------------------------------------------------------------------------------------------------|
| Language | Solidity |
| Codebase | https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty |
| Commit | [d7c4bd8](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/commit/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20) |
| Delivery Date | ____ |
|-------------------|--------------------------------|
| Audit Methodology | Manual Review, Contract Fuzzing |
| Vulnerability Level | Total | Pending | Declined | Acknowledged | Partially Resolved | Resolved |
|---------------------|-------|---------|----------|--------------|--------------------|----------|
| [Critical](#Critical)| 14 | 14 | 0 | 0 | 0 | 0 |
| [High](#High) | 7 | 7 | 0 | 0 | 0 | 0 |
| [Medium](#Medium) | 4 | 4 | 0 | 0 | 0 | 0 |
| [Low](#Low) | 6 | 6 | 0 | 0 | 0 | 0 |
# Audit Scope & Methodology
## Scope
| ID | File | SHA-1 Hash |
|---------|------------|---------------------------------------|
| CE | src/CrabEngine.sol | 14549b75f7af96eff5407a47b1743ecb3543c715 |
| CSC | src/CrabStablecoin.sol | a4da72d21bd78d84fb1c29a3cd71a3ebc9bcee07 |
| LC | src/LiquidationCallback.sol | 3139ae9c31a51490028fd99d32eddb6c65db7079 |
| CGS | src/ClawGovernanceStaking.sol | fdc8b79812b931b721846d5745609c76faa93817 |
| CGS | src/ClawGovernanceCoin.sol | 9f4b139ed04f36eee1a007381563bee4a415d426 |
## Methodology
The auditing process pays special attention to the following considerations:
- Testing the smart contracts against both common and uncommon attack vectors.
- Assessing the codebase to ensure compliance with current best practices and industry standards.
- Ensuring contract logic meets the specifications and intentions of the client.
- Cross-referencing contract structure and implementation against similar smart contracts produced by industry leaders.
- Thorough line-by-line manual review of the entire codebase by community auditors.
## Vulnerability Classifications
| Vulnerability Level | Classification |
|---------------------|----------------------------------------------------------------------------------------------|
| [Critical](#Critical) | Easily exploitable by anyone, causing causing loss of assets or undermining of the protocol’s goals. |
| [High](#High) | Arduously exploitable by a subset of addresses, causing loss of assets or undermining of the protocol’s goals. |
| [Medium](#Medium) | Inherent risk of future exploits that may or may not impact the smart contract execution. |
| [Low](#Low) | Minor deviation from best practices. |
# Findings & Resolutions
| ID | Title | Category | Severity | Status |
|-------|-------------------------------------------------------------------------------------------|---------------------|----------|---------|
| [C-01](#C01) | User Can Only Borrow Less Than Collateral Value - Decimals Off | Logic Error | CRITICAL | Pending |
| [C-02](#C02) | Incorrect Formula for Calculating Interest | Logic Error | CRITICAL | Pending |
| [C-03](#C03) | Repay Flow Reverts With Direct Call by User in Attempts to Repay | Logic Error | CRITICAL | Pending |
| [C-04](#C04) | Incorrect Calculation of liquidationReward | Logic Error | CRITICAL | Pending |
| [C-05](#C05) | Calculation for collateralLiquidated is Incorrect | Logic Error | CRITICAL | Pending |
| [C-06](#C06) | Incorrect Formula for Liquidation | Logic Error | CRITICAL | Pending |
| [C-07](#C07) | Incorrect Formula for Governance Tokens Supply | Logic Error | CRITICAL | Pending |
| [C-08](#C08) | Incorrectly Counting Yes and No Votes Towards Voting Power | Logic Error | CRITICAL | Pending |
| [C-09](#C09) | Incorrect Calculation of accruedRewards | Logic Error | CRITICAL | Pending |
| [C-10](#C10) | Proposals Cannot Compile For Deployment | Logic Error | CRITICAL | Pending |
| [C-11](#C11) | Vote and Execute Function Doesn't Check If the Proposal Exists | Logic Error | CRITICAL | Pending |
| [C-12](#C12) | User Does Not Receive stClaw After Staking | Logic Error | CRITICAL | Pending |
| [C-13](#C13) | ClawGovernanceStaking Contract Does Not Require Return of stClaw from User | Logic Error | CRITICAL | Pending |
| [C-14](#C14) | Fees Not Updated In Withdraw Function | Logic Error | CRITICAL | Pending |
| [H-01](#H01) | Fails to Factor in Multiple Coin Collateral Portfolio in Withdraw | Logic error | HIGH | Pending |
| [H-02](#H02) | Failure to Factor in Fees in Calculating Whether User Can Borrow | Logic Error | HIGH | Pending |
| [H-03](#H03) | User Can Withdraw More Than They Are Allowed, Violates The LTV Ratio Intended | Logic Error | HIGH | Pending |
| [H-04](#H04) | Incorrect Require Statement in clawGovernanceCoin | Logic Error | HIGH | Pending |
| [H-05](#H05) | No Zero Address Checks in Propose() Function | Logic Error | HIGH | Pending |
| [H-06](#H06) | No Checks For Valid Collateral Token in ClawGovernanceCoin Contract | Logic Error | HIGH | Pending |
| [H-07](#H07) | noVotes is always 0 | Logic Error | HIGH | Pending |
| [M-01](#M01) | Ownable Instead of Ownable2Step Used | Access Controls | MEDIUM | Pending |
| [M-02](#M02) | Repay Function Makes User Guess How Much They Can Repay And Then Revert If Not Exact Amount | Logic Error | MEDIUM | Pending |
| [M-03](#M03) | Lack of Usage of Variable in Storage | Logic Error | MEDIUM | Pending |
| [M-04](#M04) | Centralization Risk | Access Control | MEDIUM | Pending |
| [L-01](#L01) | Use Custom Errors Instead Of Require Statements | Gas Optimization | LOW | Pending |
| [L-02](#L02) | Confusing Naming of TvlRatio, Intended LtvRatio | Naming Conventions | LOW | Pending |
| [L-03](#L03) | Transfer After State Changes | Logic Error | LOW | Pending |
| [L-04](#L04) | Move Functions to New Contract For ClawGovernanceCoin.sol | Best Practices Violation | LOW | Pending |
| [L-05](#L05) | Transfer After Affecting State in ClawGovernanceCoin Contract | Best Practices Violation | LOW | Pending |
| [L-06](#L06) | Lack of NatSpec Comments | Best Practices Violation | LOW | Pending |
## <a id="Critical"></a>Critical
### <a id="C01"></a> C-01: User Can Only Borrow Less Than Collateral Value, Violates LTV Intention on All Collateral
[src/CrabEngine.sol#L329](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L315)
[src/CrabEngine.sol#L426-434](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L426-L434)
#### PoC:
```solidity=
function testBorrow() public {
// deposit funds
MockERC20(weth).mint(user, amountOfWethCollateral);
vm.startPrank(user);
MockERC20(weth).approve(address(crabEngine), amountOfWethCollateral);
crabEngine.depositCollateral(weth, amountOfWethCollateral); // 15e18
console.log("Amount of WETH Collateral Deposited", amountOfWethCollateral);
// test first borrow
vm.warp(block.timestamp + 12 seconds);
uint256 borrowAmount = 1e18;
crabEngine.borrow(borrowAmount);
assertEq(crabStableCoin.balanceOf(user), borrowAmount);
vm.warp(block.timestamp + 12 seconds);
crabEngine.borrow(borrowAmount);
assertEq(crabStableCoin.balanceOf(user), 2 * borrowAmount);
console.log("borrowed Amount:", borrowAmount);
vm.warp(block.timestamp + 12 seconds);
uint256 maximumBorrow = crabEngine.getTotalBorrowableAmount();
vm.expectRevert("Amount exceeds collateral borrow value");
crabEngine.borrow(maximumBorrow);
console.log("Maximum Borrow amount:", maximumBorrow);
console.log("Calculate Fee For Position:", crabEngine._calculateFeeForPosition(user));
console.log("Calculate UserOwedBalance:", crabEngine.getUserOwedAmount());
vm.stopPrank();
}
```
Run `forge t --match-test testBorrow() -vv`
#### Description:
In the borrow function, `getTotalBorrowableAmount()` is called. Within this function, `getPriceInUSDForTokens` is called and is intended to return the uint256 total amount allowed to be borrowed per user. However, when testing the `testBorrow()` function in the `CrabEngine.t.sol` and looking at the return values compared to the WETH Collateral Deposited, users are able to borrow 429e18 in addition to the 2e18 stablecoins already borrowed. This is critical as the calculation is off and not adhering to the LTVs intended. It means that the debt tvl ratio for all of Crab is at risk if a user cannot actually borrow 70% of their deposited collateral.
Based on 15e18 WETH deposit, it would be expected that 70% of the current USD value of 15e18 WETH would be available to be borrowed. However, current price of WETH is 2,233.88. Multipled by 15 = 33,598 dollars. 70% of 33,598 dollars is 23,518 dollars of borrowable Crab stablecoin. The test returns that 429e18 is the maximum borrowable amount for the user.
This is not true and limits the utility of the protocol.
#### Recommendation:
Be explicit about how you are handling decimals in the protocol and double check calculations for decimal rounding errors and formulas for correctness.
### <a id="C02"></a> C-02: Incorrect Formula for Calculating Interest
[src/CrabEngine.sol#L480-487](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L480-L487)
Affects the Following Functions: [`liquidate()#L186`](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L186), [`borrow()#L336`](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L336), [`getUserOwedAmount#L441`](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L441)
#### PoC:
```solidity=
function testRepayWithBorrowedAmountRepaid() public {
uint256 amountOfCrabTheUserWantsToBorrow = 2e18;
// Mint some tokens for the user 15e18
MockERC20(weth).mint(user, amountOfWethCollateral);
// Approve the CrabEngine contract to spend the user's tokens
vm.startPrank(user);
MockERC20(weth).approve(address(crabEngine), amountOfWethCollateral);
// transfer the user's tokens to the crabengine
crabEngine.depositCollateral(weth, amountOfWethCollateral);
vm.stopPrank();
// Check if the deposit was successful
uint256 userCollateralDeposited = crabEngine.s_collateralDeposited(user, weth);
assertEq(userCollateralDeposited, amountOfWethCollateral);
// at this point the user has collateral, now they need to borrow some crab
vm.startPrank(user);
crabEngine.borrow(amountOfCrabTheUserWantsToBorrow);
vm.stopPrank();
// now the user should have their crab, let's check if they have it
uint256 amountOfCrabBorrowed = crabEngine.getUserCrabBalance(user);
assertEq(amountOfCrabBorrowed, amountOfCrabTheUserWantsToBorrow);
vm.warp(block.timestamp + 20 seconds);
// // user withdraws the collateral
vm.startPrank(user);
uint256 fees = crabEngine._calculateFeeForPosition(user);
console.log("Fees:", fees);
uint256 repayAmount = amountOfCrabTheUserWantsToBorrow + fees;
vm.expectRevert();
crabEngine.repay(repayAmount);
}
```
As you can see, the fees are much more than the borrowed amount of 2e18.
#### Description:
`borrowedAmount * (block.timestamp - s_userBorrows[user].lastPaidAt) * INTEREST_PER_SHARE_PER_SECOND (is actually InterestPerSharePerSecondsInAYear)` is not correct and results in a fee that is likely unable to be paid and really expensive for the user. For example, if a user has borrowed `1000e18` of Crab * `1680220800` (block.timestamp for example) - `1680220000` (last Paid at event timestamp) * 3170979198 (INTEREST_PER_SHARE_PER_SECOND) = `1680220799994672057331936440000`, which is `1680220799994e18` in debt for `1000e18` borrowed. The user will not be approved to spend that many Crab StableCoin and therefore it will revert and the user will not be able to withdraw their funds.
This fee calculation isn't correct and would result in users not being able to repay their borrowedAmount and losing their collateral to innumerable fees and possible liquidation of healthy LTV positions.
#### Recommendation:
If the intended amount of interest per second is 5% over one year, then the correct formula is borrowedAmount * (secondsPassed * interestPerSharePerSecond).
### <a id="C03"></a> C-03: Repay Flow Reverts With Direct Call by User In Attempts to Repay
[src/CrabEngine.sol#L354-383](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L354-L383)
#### PoC:
```solidity=
function testWithdrawCollateralWithBorrowedAmountRepaid() public {
uint256 amountOfCrabTheUserWantsToBorrow = 2e18;
// Mint some tokens for the user 15e18
MockERC20(weth).mint(user, amountOfWethCollateral);
// Approve the CrabEngine contract to spend the user's tokens
vm.startPrank(user);
MockERC20(weth).approve(address(crabEngine), amountOfWethCollateral);
// transfer the user's tokens to the crabengine
crabEngine.depositCollateral(weth, amountOfWethCollateral);
vm.stopPrank();
// Check if the deposit was successful
uint256 userCollateralDeposited = crabEngine.s_collateralDeposited(user, weth);
assertEq(userCollateralDeposited, amountOfWethCollateral);
// at this point the user has collateral, now they need to borrow some crab
vm.startPrank(user);
crabEngine.borrow(amountOfCrabTheUserWantsToBorrow);
vm.stopPrank();
// now the user should have their crab, let's check if they have it
uint256 amountOfCrabBorrowed = crabEngine.getUserCrabBalance(user);
assertEq(amountOfCrabBorrowed, amountOfCrabTheUserWantsToBorrow);
// // user withdraws the collateral
vm.warp(block.timestamp + 20 seconds);
vm.startPrank(user);
uint256 userOwedAmount = crabEngine.getUserOwedAmount();
deal(address(crabStableCoin), user, userOwedAmount);
console.log("User Owed Amount:", userOwedAmount);
vm.expectRevert();
crabStableCoin.approve(address(crabEngine), userOwedAmount);
crabEngine.repay(userOwedAmount);
crabEngine.withdrawCollateral(weth, amountOfWethCollateral);
}
```
#### Description:
User cannot repay `userOwedAmount` (which includes borrowed Crab and fees) directly due to lack of ERC20 allowance for the Crab StableCoin for the user. The protocol is required to mint them Crab to pay the fees.
It is unclear if the protocol will manually mint more Crab StableCoin to repay this debt for the user or how this is handled in the protocol for the end user. It is also unclear if they user needs to call the `getUserOwedAmount()` function before getting the Crab in fees and repaying their loan to then withdraw their collateral.
If user repays the amount that they borrowed, the flow is confusing. Does the user need to request repayment minting from the Crab owner?
#### Recommendation:
Allow the user to call `repay()` and repay their whole debt and then withdrawal their collateral. Alternatively, add a diagram of intended user flow in the README.md for other developers and users to review.
### <a id="C04"></a> C-04: Incorrect Calculation of liquidationReward
#### PoC:
Scenario:
User borrows 100 Crab. The LIQUIDATION_REWARD is set to 500. In this case, the total to be liquidated according to line 210 in CrabEngine.sol#L210 is 100 Crab * 500 / 100. In other words, there would be 500 Crab (in e18) needed to be liquidated in addition to the amount of Crab borrowed. The user will not have this amount of crab resulting a revert for the liquidate function, preventing liquidations from occuring properly.
[src/CrabEngine.sol#L210](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L210)
#### Description:
```solidity=210
uint256 liquidationReward = amountOfCrabBorrowed * LIQUIDATION_REWARD / 100;
```
On line 210 in the CrabEngine.sol, the liquidationReward is not calculated properly. The amount of crab or liquidity required to reward the liquidator is not in possession of the user for the transfer taking place on line 239.
```solidity=239
IERC20(s_typesOfCollateralTokens[i]).safeTransfer(msg.sender, liquidationReward)
```
[src/CrabEngine.sol#L239](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L210)
#### Recommendation:
Correctly calculate the liquidation reward according to the interestPerSharePerSecond formula, then transfer this amount (5%) to the liquidator from the user.
### <a id="C05"></a> C-05: Calculation for collateralLiquidated is Incorrect
Relevant Links: [CrabEngine.sol#L211](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L211), [CrabEngine.sol#L219](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L219), [CrabEngine.sol#L220-222](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L220-222), [CrabEngine.sol#L234](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L234)
#### Description:
The calculation on the collateralToLiquidateInToken is not correct. It affects the collateralLiquidated such that the amount of collateral to liquidate is greater than the amount of user balance. This is happening in part because of C-04. The liquidationReward is not calculated properly and the collateralToLiquidate = amountOfCrabBorrowed + liquidationReward, so the user will never have the amount of collateral to be liquidated.
### Recommendation:
Correct the calculation with the liquidationReward to be accurately 5% of the borrowed Crab.
### <a id="C06"></a> C-06: Incorrect Formula for Liquidation
[src/CrabEngine.sol#L232](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L232)
```solidity=232
i_crabStableCoin.transferFrom(msg.sender, address(this), price);
```
#### Description:
Price should not be a factor in this transferFrom. It should be the amount of Crab needed to liquidate the position the user has. It should also not loop through all the collateral.
#### Recommendation:
Do not loop through all the collateral. Do calculate the amount of crab borrowed and needed to liquidate the position.
### <a id="C07"></a> C-07: Incorrect Formula for Governance Tokens Supply
[src/CrabEngine.sol#L186-188](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L186-L188)
```solidity=186
function getTotalSupplyOfGovernanceTokens() private view returns (uint256) {
return totalSupply() + stakingContract.getTotalStakedAmount();
}
```
```solidity=53
require(totalVotes > getTotalSupplyOfGovernanceTokens() / 2, "Proposal has not reached more than 50% of the total supply in voting power");
```
```solidity=50
uint256 onePercentTotalSupplyOfToken = (totalSupply() + stakingContract.getTotalStakedAmount()) / 100;
```
#### Description:
The amount of governance tokens is counted twice here with the totalSupply() plus the totalStakedAmount(), however the staked amount is included in the totalSupply() resulting in double counting the governance tokens. This affects the staking of users tokens and ability to vote on proposals. This directly affects control over the ltv's for the protocol.
#### Recommendation:
Do not double count the total supply of governance tokens. Count them once. Refactor the onePercentTotalSupplyOfToken variable to only count the totalSupply / 100, not the totalSupply plus the staked amount of Claw.
### <a id="C08"></a> C-08: Incorrectly Counting Yes and No Votes Towards Voting Power
```solidity=156
uint256 totalVotes = proposal.yesVotes + proposal.noVotes;
```
```solidity=158-161
require(
totalVotes > getTotalSupplyOfGovernanceTokens() / 2,
"Proposal has not reached more than 50% of the total supply in voting power"
);
```
#### Description:
Counting both yes and no votes as in favor of the proposal and executing it. Not differentiating between the yesVotes and noVotes. The second code sample is intended to see if the totalVotes is greater than 50% of the voting power, but if the intention is to have noVotes counting as down votes to a proposal, then they are currently not performing that action. They are instead being counted in addition to yesVotes towards totalVotes. This is not the intended functionality and affects the governance of the protocol.
#### Recommendation:
Do not count noVotes in line 156 or eliminate noVotes functionality.
### <a id="C09"></a> C-09: Incorrect Calculation of accruedRewards
```solidity=49
stakes[msg.sender].accruedRewards += interest * stakes[msg.sender].stakeAmount / i_clawGovernanceCoin.balanceOf(address(this));
```
[src/CrabEngine.sol#L49](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceStaking.sol#L49)
#### Description:
The accruedRewards will always equal 0. No rewards will be accrued for stakers. This is unexpected behavior for the contract and violates the core assumption users have that rewards will be accrued as expected.
#### Recommendation:
Fix formulas for interest/accruedRewards.
### <a id="C10"></a> C-10: Proposals Cannot Compile For Deployment
#### PoC
#### Description:
proposalId is not instantiated in the storage.
[ClawGovernanceCoin.sol#L55](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L55-L68):
```solidity=55
proposalId = proposalCount++;
proposals[proposalId] = Proposal({
executed: false,
proposer: msg.sender,
token: token,
ltv: ltv,
yesVotes: 0,
noVotes: 0,
endTime: block.timestamp + 5 days
});
// Automatically vote for the proposal with the entire balance
vote(proposalId);
```
#### Recommendation:
Use the proposalCount variable instead of proposalId, which is defined in storage and never used.
### <a id="C11"></a> C-11: Vote and Execute Function Doesn't Check If the Proposal Exists
#### Description:
Vote and Execute function doesn't check if the proposal exists. This could lead to duplicate proposals and unintended functionality for voting.
[ClawGovernanceCoin.sol#L55-L67](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L55-L67):
```solidity=55
proposalId = proposalCount++;
proposals[proposalId] = Proposal({
executed: false,
proposer: msg.sender,
token: token,
ltv: ltv,
yesVotes: 0,
noVotes: 0,
endTime: block.timestamp + 5 days
});
// Automatically vote for the proposal with the entire balance
vote(proposalId);
```
#### Recommendation:
Check if the proposal exists to be voted on in storage for both functions. Make a modifier to check the proposal exists.
### <a id="C12"></a> C-12: User Does Not Receive stClaw After Staking
User does not receive stClaw after Staking.
#### Description:
The stake function in ClawGovernanceStaking contract does not transfer the stClaw to the user. It keeps the stakedAmount internally. This is not expected functionality.
#### Recommendation:
Send the stClaw to the user.
### <a id="C13"></a> C-13: ClawGovernanceStaking Contract Does Not Require Return of stClaw from User
#### Description:
Unstake functionality does not currently require the user to send in their stClaw to receive Claw.
#### Recommendation:
Transfer the stClaw amount user has a balanceOf to the ClawGovernanceStaking contract or burn it, then return the Claw governance token amount plus the rewards accrued.
### <a id="C14"></a> C-14: Fees Not Updated In Withdraw Function
[src/CrabEngine.sol#L281-305](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L281-L305)
#### Description:
In the withdraw function, the fees for the protocol are not updated. This is a problem because the overall tvl is not tracked and the fees for the protocol cannot be distributed to stakeholders accurately if it is not updated in the appropriate places when users interact with the CrabEngine contract.
#### Recommendation:
Update the fees in the withdraw function.
----
## <a id="High"></a> High
### <a id="H01"></a> H-01 Fails to Factor in Multiple Coin Collateral Portfolio in Withdraw
[src/CrabEngine.sol#L292-397](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L292-L297)
#### PoC:
User cannot pass multiple collateral types to the withdraw function.
```solidity=201
function withdrawCollateral(
address collateralTokenAddress,
uint256 amount
)
```
#### Description:
The contract doesn't account for multiple coin portfolios in the calculation of withdrawing user collateral in the case that user has already borrowed some Crab. Only takes into account one type of coin, despite accepting SOL, WETH, and USDC with different LTVs. Only allows withdraw of one coin at a time.
#### Recommendation:
Factor in all the collateral in calculations for withdraw and the full portfolio and fix the totalBorrowableAmount function to report the appropriate amount of borrowable collateral.
### <a id="H02"></a> H-02 Failure to Factor in Fees in Calculating Whether User Can Borrow
[src/CrabEngine.sol#L332-L334](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L332-L334)
#### PoC:
Scenario:
User requests to borrow 2e18 Crab. User already deposited 15e6 USDC. User has previously borrowed 1e18 Crab. LTV for USDC is 80%. User should be able to borrow up to 12 USDC or 12e18 Crab. They should be able to borrow 11e18 Crab, but not 12e18 Crab, if correctly calculated. This would result in a revert.
#### Description:
`require(amount + s_userBorrows[msg.sender].borrowAmount < maxBorrow, "Amount exceeds collateral borrow value");`
The borrow function, when borrowedAmount is not zero is not factoring in fees to decide if user can borrow or not, according to LTV. It is unclear where the fees are coming from in the protocol.
#### Recommendation:
Add correctly calculated 5% interestPerSharePerSecond fees to this require statement.
### <a id="H03"></a> H-03: Incorrect Require Statement in clawGovernanceCoin
[ClawGovernanceStaking.solL#43](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceStaking.sol#L43)
```solidity=43
require(i_clawGovernanceCoin.balanceOf(msg.sender) >= amount, "Not enough funds");
```
#### Description:
This is wrong. If it is great than or equal to the amount requested to be staked, then it should be staked no problem because there is enough of the GovernanceCoin in the user's balance to stake. Will create revert situations for users eligible to stake. It could be critical for governance contracts.
#### Recommendation:
Should be `require(i_clawGovernanceCoin.balanceOf(msg.sender) < amount, "Not enough funds");`
### <a id="H04"></a> H-04: No Zero Address Checks in Propose() Function
#### Description:
User could use address(0) and suggest an ltv in a proposal.
#### Recommendation:
Do zero address checks.
### <a id="H05"></a> H-05: No Checks For Valid Collateral Token in ClawGovernanceCoin Contract
#### PoC
INSERT HERE
#### Description:
[ClawGovernanceCoin#L48-68](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L48-L68)
There are no checks for valid collateral types in ClawGovernanceCoin contract within the propose() function.
#### Recommendation:
Do checks to see if the token is already supported or not to see if it is a valid collateral type. With the address input from the user, it may not be a supported collateral type.
### <a id="H06"></a> H-06: noVotes is always 0
[ClawGovernanceCoin#L48-68](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L48-L68)
PoC:
Scenario: If a user votes, attempts to vote no, then they are not able to.
#### Description:
noVotes is always going to be 0 within the execute() in the ClawGovernanceCoin.sol contract. i.e. cannot vote no on a proposal. If you do vote no currently, it counts towards the total votes for executing it.
#### Recommendation:
Do not count noVotes, only allow yesVotes. Eliminate the functionality for noVotes or implement it as decrementing the total voting power, instead of incrementing.
-----------------
## <a id="Medium"></a> Medium
### <a id="M01"></a> M-01 Ownable Instead of Ownable2Step
[src/CrabEngine.sol#L29](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L29)
[src/ClawGovernanceCoin.sol#L9](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceCoin.sol#L9)
[src/ClawGovernanceStaking.sol#L10](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceStaking.sol#L10)
[src/CrabStableCoin.sol#L18](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabStableCoin.sol#L18)
#### Description:
Risk of owner transferring ownership to an owner who cannot accept ownership or is transferred ownership by mistake due to not using Ownable2Step.
#### Recommendation:
Use Ownable2Step instead of Ownable OpenZeppelin Library.
### <a id="M02"></a> M-02 Repay Function Makes User Guess How Much They Can Repay And Then Revert If Not Exact Amount
[src/CrabEngine.sol#L361-368](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L361-L368)
#### Description:
Repay function makes user guess how much they can repay and then revert if not exact amount. Could gas grief the user by miscalculating the amount able to be repaid, if user needs to do multiple attempts to try to repay their amount.
#### Recommendation:
Allow user to have another public function that tells them how much to repay to withdraw their full collateral amount.
### <a id="M03"></a> M-03 Lack of Usage of Variable in Storage
```solidity=337
s_protocolDebtInCrab += amount
```
[CrabEngine.sol#L339](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L339)
#### Description:
The contracts do not use this variable for tracking the total protocol debt properly.
#### Recommendation:
Add appropriate fees to this amount, if decided to implement a variable that keeps track of the protocol debt in Crab coin.
### <a id="M04"></a> M-04 Centralization Risk
Examples of onlyOwner modifier usage within CrabEngine.sol:
[CrabEngine.sol#L400](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L400), [CrabEngine.sol#L409](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L409), [CrabEngine.sol#L418](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L418)
#### Description:
Only the owner can mint, burn, add coins and feeds, and set the Staking contract address.
#### Recommendation:
Make the owner a multisig. This way these functions require more permissions than a singular point of failure for the admin.
-----------------
## <a id="Low"></a> Low
### <a id="L01"></a> L-01 Use Custom Errors Instead Of Require Statements
Ex: [src/CrabEngine.sol#L319](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L319)
#### Description:
Right now, the Crab Protocol uses a combination of requires and custom errors.
#### Recommendation:
Consider using only custom errors to save on gas and increase the readability of the code for other developers.
### <a id="L02"></a> L-02 Confusing Naming of TvlRatio, Intended LtvRatio
Ex: [src/CrabEngine.sol#L150](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L150)
#### Description:
Right now, the variable is named tvlRatio, but seems to track the ltvRatio instead.
#### Recommendation:
Update name of variable to avoid confusion of other developers.
### <a id="L03"></a> L-03 Transfer After State Changes
```solidity=375
bool success = i_crabStableCoin.transferFrom(msg.sender, address(this), amount);
if (!success) {
revert CrabEngine__TransferFailed();
}
```
[src/CrabEngine.sol#L375-378](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L375-L378)
#### Description:
Right now, this is at not at the beginning of the function resulting in performing state changes before collecting the coins from the msg.sender.
#### Recommendation:
This should be at the beginning of the function before state changes.
### <a id="L04"></a> L-04 Move Functions to New Contract
[src/CrabEngine.sol#L375-378](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/CrabEngine.sol#L375-L378)
#### Description:
Right now, the functions within the ClawGovernanceCoin.sol do not belong in that contract, rather they belong in a new contract for governance. The ClawGovernanceCoin.sol is meant to be just the ERC20.
#### Recommendation:
Move the following functions in GovernanceCoin to a new contract: vote, propose, and execute.
### <a id="L05"></a> L-05 Transfer After Affecting State
```solidity=56
i_clawGovernanceCoin.transferFrom(msg.sender, address(this), amount);
```
[ClawGovernanceStaking.sol#L56](https://github.com/owenThurm/crab-stablecoin-maxgrok-sheepghosty/blob/d7c4bd8a7ed01ea0c3b707983f76d3da9b95ba20/src/ClawGovernanceStaking.sol#L56)
#### Description:
Changing a lot of state before taking the coin is not advisable.
#### Recommendation:
The protocol should do this first in the `stake()` function.
### <a id="L06"></a> L-06 Lack of NatSpec Comments
#### Description:
The contracts are missing comprehensive NatSpec comments for ClawGovernanceStaking Contract.
#### Recommendation:
Add NatSpec to the ClawGovernanceStaking contract codebase.
____
## Disclaimer
>
> This report is not, nor should be considered, an “endorsement” or “disapproval” of any particular project or team. This report is not, nor should be considered, an indication of the economics or value of any “product” or “asset” created by any team or project that contracts the firm to perform a security assessment. This report does not provide any warranty or guarantee regarding the absolute bug-free nature of the technology analyzed, nor do they provide any indication of the technologies proprietors, business, business model or legal compliance.
>
> This report should not be used in any way to make decisions around investment or involvement with any particular project. This report in no way provides investment advice, nor should be leveraged as investment advice of any sort. This report represents an extensive assessing process intending to help our customers increase the quality of their code while reducing the high level of risk presented by cryptographic tokens and blockchain technology.
>
> Blockchain technology and cryptographic assets present a high level of ongoing risk. The firm’s position is that each company and individual are responsible for their own due diligence and continuous security. The firm’s goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies, and in no way claims any guarantee of security or functionality of the technology we agree to analyze.
>
> The assessment services provided by the firm is subject to dependencies and under continuing
> development. You agree that your access and/or use, including but not limited to any services, reports, and materials, will be at your sole risk on an as-is, where-is, and as-available basis. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. The assessment reports could include false positives, false negatives, and other unpredictable results. The services may access, and depend upon, multiple layers of third-parties.
>
> Notice that smart contracts deployed on the blockchain are not resistant from internal/external exploit. Notice that active smart contract owner privileges constitute an elevated impact to any smart contract’s safety and security. Therefore, the firm does not guarantee the explicit security of the audited smart contract, regardless of the verdict.
<br/>
Sign in with Wallet
Connect another wallet