Platform: Code4rena
Start Date: 21/10/2021
Pot Size: $80,000 ETH
Total HM: 28
Participants: 15
Period: 7 days
Judge: ghoulsol
Total Solo HM: 19
Id: 42
League: ETH
Rank: 2/15
Findings: 9
Award: $13,651.80
🌟 Selected for report: 12
🚀 Solo Findings: 1
WatchPug
In the current implementation, new borrows will be charged a 0.5% interest right away. Making the borrower be recorded a 100.5% amount of debt.
However, when a position got liquidated, the unrealized interest may still remain in the position while the collateral already got seized because triggerLiquidation will only reduce the debt of the position by the amount of vault.currentDebt(_nftId), and currentDebt excludes the unrealized part of the pre-charged interest.
function triggerLiquidation(address _asset, uint256 _nftId) external override { IMochiVault vault = engine.vaultFactory().getVault(_asset); Auction storage auction = auctions[auctionId(_asset, _nftId)]; require(auction.startedAt == 0 || auction.boughtAt != 0, "on going"); uint256 debt = vault.currentDebt(_nftId);
function currentDebt(uint256 _id) public view override returns (uint256) { require(details[_id].status != Status.Invalid, "invalid"); uint256 newIndex = liveDebtIndex(); return (details[_id].debt * newIndex) / details[_id].debtIndex; }
Furthermore, when this happens, the position will actually remain liquidatable unexpectedly and can be liquidated again. It will be pretty much just a waste of gas through.
Given:
If the price of the collateral asset goes down, and Alice's position got liquidated right after.
The debt of Alice's position will now be 5 USDM, while all the collateral assets got seized and the collateral is now 0.
If Alice deposits to this position again rather than create a new position, this unfair amount of debt will be added to the position.
Change to:
function triggerLiquidation(address _asset, uint256 _nftId) external override { IMochiVault vault = engine.vaultFactory().getVault(_asset); Auction storage auction = auctions[auctionId(_asset, _nftId)]; require(auction.startedAt == 0 || auction.boughtAt != 0, "on going"); uint256 currentDebt = vault.currentDebt(_nftId); (, uint256 collateral, uint256 debt, , ) = vault.details(_nftId); vault.liquidate(_nftId, collateral, debt); auction.nftId = _nftId; auction.vault = address(vault); auction.startedAt = block.number; auction.boughtAt = 0; auction.collateral = collateral; auction.debt = currentDebt; uint256 liquidationFee = currentDebt.multiply( engine.mochiProfile().liquidationFee(address(_asset)) ); emit Triggered(auctionId(_asset, _nftId), currentDebt + liquidationFee); }
#0 - r2moon
2021-10-29T16:53:13Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/68
#1 - ghoul-sol
2021-11-02T01:19:03Z
duplicate of #72
🌟 Selected for report: WatchPug
0.9718 ETH - $4,045.16
WatchPug
distributeMochi() will call _buyMochi() to convert mochiShare to Mochi token and call _shareMochi() to send Mochi to vMochi Vault and veCRV Holders. It wont touch the treasuryShare.
However, in the current implementation, treasuryShare will be reset to 0. This is unexpected and will cause the protocol fee can not be properly accounted for and collected.
function _shareMochi() internal { IMochi mochi = engine.mochi(); uint256 mochiBalance = mochi.balanceOf(address(this)); // send Mochi to vMochi Vault mochi.transfer( address(engine.vMochi()), (mochiBalance * vMochiRatio) / 1e18 ); // send Mochi to veCRV Holders mochi.transfer( crvVoterRewardPool, (mochiBalance * (1e18 - vMochiRatio)) / 1e18 ); // flush mochiShare mochiShare = 0; treasuryShare = 0; }
Anyone can call distributeMochi() and reset treasuryShare to 0, and then call updateReserve() to allocate part of the wrongfuly resetted treasuryShare to mochiShare and call distributeMochi().
Repeat the steps above and the treasuryShare will be consumed to near zero, profits the vMochi Vault holders and veCRV Holders. The protocol suffers the loss of funds.
Change to:
function _buyMochi() internal { IUSDM usdm = engine.usdm(); address[] memory path = new address[](2); path[0] = address(usdm); path[1] = address(engine.mochi()); usdm.approve(address(uniswapRouter), mochiShare); uniswapRouter.swapExactTokensForTokens( mochiShare, 1, path, address(this), type(uint256).max ); // flush mochiShare mochiShare = 0; } function _shareMochi() internal { IMochi mochi = engine.mochi(); uint256 mochiBalance = mochi.balanceOf(address(this)); // send Mochi to vMochi Vault mochi.transfer( address(engine.vMochi()), (mochiBalance * vMochiRatio) / 1e18 ); // send Mochi to veCRV Holders mochi.transfer( crvVoterRewardPool, (mochiBalance * (1e18 - vMochiRatio)) / 1e18 ); }
0.4373 ETH - $1,820.32
WatchPug
function claimRewardAsMochi() external { IUSDM usdm = engine.usdm(); address[] memory path = new address[](2); path[0] = address(usdm); path[1] = uniswapRouter.WETH(); path[2] = address(engine.mochi()); usdm.approve(address(uniswapRouter), reward[msg.sender]); // we are going to ingore the slippages here uniswapRouter.swapExactTokensForTokens( reward[msg.sender], 1, path, address(this), type(uint256).max );
In ReferralFeePoolV0.sol#claimRewardAsMochi(), path is defined as an array of length 2 while it should be length 3.
As a result, at L33, an out-of-bound exception will be thrown and revert the transaction.
claimRewardAsMochi() will not work as expected so that all the referral fees cannot be claimed but stuck in the contract.
🌟 Selected for report: harleythedog
Also found by: WatchPug
WatchPug
MochiVault.sol#withdraw() is using the wait() modifier to prevent withdraw within delay duration from lastDeposit.
However, MochiVault.sol#deposit() allows anyone to deposit to a specific position. This enables the attacker to initiate a griefing attack by depositing 0 to the target's position and preventing the target to withdraw from the position for engine.mochiProfile().delay().
modifier wait(uint256 _id) { require( lastDeposit[_id] + engine.mochiProfile().delay() <= block.timestamp, "!wait" ); accrueDebt(_id); _; }
Consider making MochiVault.sol#deposit() limited to the owner of the positions.
#0 - r2moon
2021-10-29T15:28:26Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/69
WatchPug
In the current implementation, a liquidated position can be used for depositing and borrowing again.
However, if there is a liquidation auction ongoing, even if the position is now liquidatable, the call of triggerLiquidation() will still fail.
The liquidator must settleLiquidation first.
If the current auction is not profitable for the liquidator, say the value of the collateral can not even cover the gas cost, the liquidator may be tricked and not liquidate the new loan at all.
Considering if the liquidator bot is not as small to handle this situation (take the profit of the new liquidation and the gas cost loss of the current auction into consideration), a malicious user can create a dust amount position trigger the liquidation by themself.
Since the collateral of this position is so small that it can not even cover the gas cost, liquidators will most certainly ignore this auction.
The malicious user will then deposit borrow the actual loan.
When this loan becomes liquidatable, liquidators may:
liquidatable position;As a result, the malicious user can potentially escape liquidation.
Consider making liquidated positions unable to be used (for depositing and borrowing) again.
WatchPug
ERC20 implementations are not always consistent. Some implementations of transfer could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures. Unsafe transfer calls were found in the following locations:
asset.transfer(msg.sender, _collateral);
Check the return value and revert on 0/false or use OpenZeppelin’s SafeERC20 wrapper functions.
#0 - r2moon
2021-10-27T15:55:59Z
🌟 Selected for report: nikitastupin
WatchPug
function getPrice(address _asset) public view override returns (float memory) { (, int256 price, , , ) = feed[_asset].latestRoundData(); uint256 decimalSum = feed[_asset].decimals() + IERC20Metadata(_asset).decimals(); if (decimalSum > 18) { return float({ numerator: uint256(price), denominator: 10**(decimalSum - 18) }); } else { return float({ numerator: uint256(price) * 10**(18 - decimalSum), denominator: 1 }); } }
On ChainlinkAdapter.sol, we are using latestRoundData, but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:
Consider adding missing checks for stale data.
For example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = feed[_asset].latestRoundData(); require(price > 0, "Chainlink price <= 0"); require(answeredInRound >= roundID, "Stale price"); require(timeStamp != 0, "Round not complete");
#0 - r2moon
2021-10-27T15:31:46Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/87
WatchPug
treasuryRatio and vMochiRatio must be <= 1e18 to make sure the contract works correctly. Therefore, the input should be checked in the setters.
function changeTreasuryRatio(uint256 _ratio) external { require(msg.sender == engine.governance(), "!gov"); treasuryRatio = _ratio; } function changevMochiRatio(uint256 _ratio) external { require(msg.sender == engine.governance(), "!gov"); vMochiRatio = _ratio; }
Change to:
function changeTreasuryRatio(uint256 _ratio) external { require(msg.sender == engine.governance(), "!gov"); require(_ratio <= 1e18, ">1e18"); treasuryRatio = _ratio; } function changevMochiRatio(uint256 _ratio) external { require(msg.sender == engine.governance(), "!gov"); require(_ratio <= 1e18, ">1e18"); vMochiRatio = _ratio; }
🌟 Selected for report: WatchPug
WatchPug
mochi.transfer(msg.sender, _amount / 2); mochi.transfer(address(vMochi), _amount / 2);
Change to:
mochi.transfer(msg.sender, _amount / 2); mochi.transfer(address(vMochi), _amount - _amount / 2);
operationShare += updatedFee / 2; veCRVShare += updatedFee / 2;
Change to:
operationShare += updatedFee / 2; veCRVShare += updatedFee - updatedFee / 2;
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
MochiProfileV0.sol#changeAssetClass(), MochiProfileV0.sol#changeCreditCap()
function changeAssetClass( address[] calldata _assets, AssetClass[] calldata _classes ) external override onlyGov { for (uint256 i = 0; i < _assets.length; i++) { _assetClass[_assets[i]] = _classes[i]; } } function changeCreditCap( address[] calldata _assets, uint256[] calldata _caps ) external onlyGov { for (uint256 i = 0; i < _assets.length; i++) { creditCap[_assets[i]] = _caps[i]; } }
#0 - r2moon
2021-10-27T15:58:00Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/64
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
function getLiquidity(address token, address denominator) external view override returns (uint256) { IUniswapV2Pair pair = IUniswapV2Pair( UniswapV2Library.pairFor(uniswapFactory, token, denominator) ); Window memory currentWindow = window[windowIndex(block.timestamp)]; uint128 lastObserved = currentWindow.to; if (lastObserved == 0) { lastObserved = window[windowIndex(block.timestamp) - WINDOW_SIZE] .to; require(lastObserved != 0, "!observed"); } BlockData memory state = blockState[lastObserved]; require(block.timestamp - state.blockTimestamp < WINDOW_SIZE, "stale");
The initialization of pair can be done later to avoid unnecessary code execution when the check of block.timestamp - state.blockTimestamp < WINDOW_SIZE does not pass.
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
When the vesting ends, vesting[recipient].ends will be 0 which always passes the check of vesting[recipient].ends < block.timestamp and causes unnecessary code execution.
Adding a check of vesting[recipient].ends > 0 can avoid unnecessary code execution and save gas.
modifier checkClaimable(address recipient) { if (vesting[recipient].ends < block.timestamp) { vesting[recipient].claimable += vesting[recipient].vested; vesting[recipient].vested = 0; vesting[recipient].ends = 0; } _; }
Change to:
modifier checkClaimable(address recipient) { if (vesting[recipient].ends > 0 && vesting[recipient].ends < block.timestamp) { vesting[recipient].claimable += vesting[recipient].vested; vesting[recipient].vested = 0; vesting[recipient].ends = 0; } _; }
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
When liquidators race to liquidate a position, all other besides the first liquidator will be handling an empty (liquidated) position.
function triggerLiquidation(address _asset, uint256 _nftId) external override { IMochiVault vault = engine.vaultFactory().getVault(_asset); Auction storage auction = auctions[auctionId(_asset, _nftId)]; require(auction.startedAt == 0 || auction.boughtAt != 0, "on going"); uint256 debt = vault.currentDebt(_nftId); (, uint256 collateral, , , ) = vault.details(_nftId); vault.liquidate(_nftId, collateral, debt); ...
In the current implementation, even if the position is liquidated, at L77 and L79, it still tries to get the details and call vault.liquidate(), until it reverts at L285-L288 on MochiVault.sol#liquidate(). That's going to cost a decent amount of gas due to these unnecessary external calls and code executions.
function liquidate( uint256 _id, uint256 _collateral, uint256 _usdm ) external override updateDebt(_id) { require(msg.sender == address(engine.liquidator()), "!liquidator"); require(engine.nft().asset(_id) == address(asset), "!asset"); float memory price = engine.cssr().getPrice(address(asset)); require( _liquidatable(details[_id].collateral, price, currentDebt(_id)), "healthy" ); ...
Therefore, adding a precondition check can save gas.
Change to:
function triggerLiquidation(address _asset, uint256 _nftId) external override { IMochiVault vault = engine.vaultFactory().getVault(_asset); Auction storage auction = auctions[auctionId(_asset, _nftId)]; require(auction.startedAt == 0 || auction.boughtAt != 0, "on going"); uint256 debt = vault.currentDebt(_nftId); require(debt > 0, "!debt"); (, uint256 collateral, , , ) = vault.details(_nftId); vault.liquidate(_nftId, collateral, debt); ...
🌟 Selected for report: loop
Also found by: WatchPug, defsec, gzeon, harleythedog
WatchPug
Unused storage variables in contracts use up storage slots and increase contract size and gas usage at deployment and initialization.
Instances include:
uint256 public liquidated;
#0 - r2moon
2021-10-28T15:13:25Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/88
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
function accrueDebt(uint256 _id) public { // global debt for vault // first, increase gloabal debt; uint256 currentIndex = liveDebtIndex(); uint256 increased = (debts * currentIndex) / debtIndex - debts; debts += increased; claimable += int256(increased); // update global debtIndex debtIndex = currentIndex; lastAccrued = block.timestamp; // individual debt if (_id != type(uint256).max && details[_id].debtIndex < debtIndex) { require(details[_id].status != Status.Invalid, "invalid"); if (details[_id].debt != 0) { uint256 increasedDebt = (details[_id].debt * debtIndex) / details[_id].debtIndex - details[_id].debt; uint256 discountedDebt = increasedDebt.multiply( engine.discountProfile().discount(engine.nft().ownerOf(_id)) ); debts -= discountedDebt; claimable -= int256(discountedDebt); details[_id].debt += (increasedDebt - discountedDebt); } details[_id].debtIndex = debtIndex; } }
At L93, debtIndex is set as currentIndex, therefore, at L96, L99, and L109 storage variable debtIndex can be changed to local variable currentIndex to avoid unnecessary storage read to save some gas.
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
The check of x > 3 is unnecessary and most certainly adds more gas cost than it saves as the majority of use cases of this function will not be handling x <= 3.
function sqrt(uint x) internal pure returns (uint y) { if (x > 3) { uint z = x / 2 + 1; y = x; while (z < y) { y = z; z = (x / z + z) / 2; } } else if (x != 0) { y = 1; } }
Change to:
function sqrt(uint x) public pure returns (uint y) { uint z = (x + 1) / 2; y = x; while (z < y) { y = z; z = (x / z + z) / 2; } }
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
In the current implementation, withdraw(), borrow() and liquidate() in MochiVault.sol checks for engine.nft().asset(_id) == address(asset), while the assertion in deposit() already made sure that engine.nft().asset(_id) == address(asset).
Removing it will make the code simpler and save some gas.
require(engine.nft().asset(_id) == address(asset), "!asset");
require(engine.nft().asset(_id) == address(asset), "!asset");
require(engine.nft().asset(_id) == address(asset), "!asset");
🌟 Selected for report: WatchPug
0.0768 ETH - $319.56
WatchPug
In MochiProfileV0.sol, secPerYear is defined as an immutable variable while it's not configured as a parameter of the constructor. Thus, it can be declared as constant to save gas.
uint256 public immutable secPerYear; uint256 public override delay; constructor(address _engine) { secPerYear = 31536000;
Change to:
uint256 public constant SEC_PER_YEAR = 31536000;
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
reward[_recipient] += newReward; rewards += newReward;
if (debts < _amount) { // safe gaurd to some underflows debts = 0; } else { debts -= _amount; }
#0 - r2moon
2021-10-27T15:53:09Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/82
WatchPug
Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
For example:
engine.mochi() can be cached.function claimRewardAsMochi() external { IUSDM usdm = engine.usdm(); address[] memory path = new address[](2); path[0] = address(usdm); path[1] = uniswapRouter.WETH(); path[2] = address(engine.mochi()); usdm.approve(address(uniswapRouter), reward[msg.sender]); // we are going to ingore the slippages here uniswapRouter.swapExactTokensForTokens( reward[msg.sender], 1, path, address(this), type(uint256).max ); engine.mochi().transfer( msg.sender, engine.mochi().balanceOf(address(this)) ); }
engine.usdm() can be cached.function settleLiquidation( uint256 _auctionId, uint256 _collateral, uint256 _repaid ) internal { Auction storage auction = auctions[_auctionId]; require(auction.boughtAt == 0, "liquidated"); IMochiVault vault = IMochiVault(auction.vault); //repay the debt first engine.usdm().transferFrom(msg.sender, address(this), _repaid); engine.usdm().burn(_repaid); IERC20 asset = vault.asset(); auction.boughtAt = block.number; asset.transfer(msg.sender, _collateral); //transfer liquidation fee to feePool uint256 liquidationFee = currentLiquidationFee(_auctionId); engine.usdm().transferFrom( msg.sender, address(engine.feePool()), liquidationFee ); emit Settled(_auctionId, _repaid + liquidationFee); }
#0 - r2moon
2021-10-27T15:55:21Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/67
🌟 Selected for report: harleythedog
Also found by: WatchPug
WatchPug
Checks at L226-227 can be done earlier to avoid unnecessary code execution when the checks failed.
function borrow( uint256 _id, uint256 _amount, bytes memory _data ) public override updateDebt(_id) { // update prior to interaction float memory price = engine.cssr().update(address(asset), _data); float memory cf = engine.mochiProfile().maxCollateralFactor( address(asset) ); uint256 maxMinted = details[_id].collateral.multiply(cf).multiply( price ); require(engine.nft().ownerOf(_id) == msg.sender, "!approved"); require(engine.nft().asset(_id) == address(asset), "!asset");
#0 - r2moon
2021-10-27T15:27:22Z