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: 3/15
Findings: 9
Award: $13,459.47
🌟 Selected for report: 6
🚀 Solo Findings: 5
leastwood
registerAsset() is used to add assets to the Mochi Protocol. These assets have an associated asset class which represents an asset's risk factor. registerAsset() can be called by any user and abused in such a way that existing assets can have their respective risk factors reset to Sigma (the riskiest class). This ultimately affects liquidations, fees and the overall health of the system. A user could abuse this to earn fees by liquidating positions that were considered less risky but due to the abuse of this function, have now become risky.
Manual code review. Discussions with the Mochi team.
Consider adding the onlyGov modifier to the registerAsset() function and checking if an asset has already been registered before making changes.
#0 - r2moon
2021-10-27T15:50:44Z
🌟 Selected for report: leastwood
0.9718 ETH - $4,045.16
leastwood
The FeePoolV0.sol contract accrues fees upon the liquidation of undercollaterised positions. These fees are split between treasury and vMochi contracts. However, when distributeMochi() is called to distribute mochi tokens to veCRV holders, both mochiShare and treasuryShare is flushed from the contract when there are still usdm tokens in the contract.
Consider the following scenario:
FeePoolV0.sol contract contains 100 usdm tokens at an exchange rate of 1:1 with mochi tokens.updateReserve() is called to set the split of usdm tokens such that treasuryShare has claim on 20 usdm tokens and mochiShare has claim on the other 80 tokens.veCRV holder seeks to increase their earnings by calling distributeMochi() before sendToTreasury() has been called.usdm tokens are converted to mochi tokens and locked in a curve rewards pool.mochiShare and treasuryShare is set to 0 (aka flushed).updateReserve() to split the leftover 20 usdm tokens between treasuryShare and mochiShare.mochiShare is now set to 16 usdm tokens.mochi tokens to veCRV holders again and again.veCRV holders have been able to receive all tokens that were intended to be distributed to the treasury.Manual code review Discussions with the Mochi team.
Consider removing the line in FeePoolV0.sol (mentioned above), where treasuryShare is flushed.
🌟 Selected for report: leastwood
0.9718 ETH - $4,045.16
leastwood
The VestedRewardPool.sol contract is a public facing contract aimed at vesting tokens for a minimum of 90 days before allowing the recipient to withdraw their mochi. The vest() function does not utilise safeTransferFrom() to ensure that vested tokens are correctly allocated to the recipient. As a result, it is possible to frontrun a call to vest() and effectively steal a recipient's vested tokens. The same issue applies to the lock() function.
https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/emission/VestedRewardPool.sol#L36-L46 https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/emission/VestedRewardPool.sol#L54-L64
Manual code review Discussions with the Mochi team
Ensure that users understand that this function should not be interacted directly as this could result in lost mochi tokens. Additionally, it might be worthwhile creating a single externally facing function which calls safeTransferFrom(), vest() and lock() in a single transaction.
leastwood
settleLiquidation() does not validate the return value of an ERC20 transfer() call. As a result, if the call to transfer does not revert but instead returns a boolean or void value, it may be possible that a user liquidates a user's position but does not receive the associated collateral. Therefore, all interactions with asset throughout the Mochi protocol should be replaced with OpenZeppelin's SafeERC20 equivalent.
Manual code review Discussions with the Mochi team
Consider utilising OpenZeppelin's SafeERC20 library in all interactions with the asset token. In this case asset.transfer(msg.sender, _collateral) in DutchAuctionLiquidator.sol should be replaced with asset.safeTransfer(msg.sender, _collateral) at the very least.
#0 - r2moon
2021-10-29T15:23:15Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/75
🌟 Selected for report: nikitastupin
leastwood
ChainlinkAdapter.getPrice() queries a Chainlink oracle to retrieve the latest price for a given asset. However, this external call does not validate the data retrieved is fresh.
https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-cssr/contracts/adapter/ChainlinkAdapter.sol#L49 https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round
Manual code review
Consider adding the following validations to ChainlinkAdapter.getPrice().
( uint80 roundId, int256 price, , uint256 updateTime, uint80 answeredInRound ) = feed[_asset].latestRoundData(); require(price > 0, "Chainlink price <= 0"); require(updateTime != 0, "Incomplete round"); require(answeredInRound >= roundId, "Stale price");
This ensures that stale prices are not used when querying ChainlinkAdapter.sol.
#0 - r2moon
2021-10-29T12:58:00Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/87
🌟 Selected for report: leastwood
0.2915 ETH - $1,213.55
leastwood
The BeaconProxyDeployer.deploy() function is used to deploy lightweight proxy contracts that act as each asset's vault. The function does not revert properly if there is a failed contract deployment or revert from the create2 opcode as it does not properly check the returned address for bytecode. The create2 opcode returns the expected address which will never be the zero address (as is what is currently checked).
Manual code review Discussions with the Mochi team Discussions with library dev
The recommended mitigation was to update iszero(result) to iszero(extcodesize(result)) in the line mentioned above. This change has already been made in the corresponding library which can be found here, however, this needs to also be reflected in Mochi's contracts.
🌟 Selected for report: leastwood
0.2915 ETH - $1,213.55
leastwood
withdrawLock() does not prevent users from calling this function when locking has been toggled. As a result, withdraws may be made unexpectedly.
Manual code review
Consider adding require(lockCrv, "!lock"); to withdrawLock() to ensure this function is not called unexpectedly. Alternatively if this is intended behaviour, it should be rather checked that the lock has not been toggled, otherwise users could maliciously relock tokens.
🌟 Selected for report: leastwood
0.2915 ETH - $1,213.55
leastwood
MochiTreasuryV0.sol interacts with Curve's voting escrow contract to lock tokens for 90 days, where it can be later withdrawn by the governance role. However, VotingEscrow.vy does not allow contracts to call the following functions; create_lock(), increase_amount() and increase_unlock_time(). For these functions, msg.sender must be an EOA account or an approved smart wallet. As a result, any attempt to lock tokens will fail in MochiTreasuryV0.sol.
https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/VotingEscrow.vy#L418 https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/VotingEscrow.vy#L438 https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/VotingEscrow.vy#L455
Manual code review Discussions with the Mochi team
Consider updating this contract to potentially use another escrow service that enables msg.sender to be a contract. Alternatively, this escrow functionality can be replaced with an internal contract which holds usdm tokens instead, removing the need to convert half of the tokens to Curve tokens. Holding Curve tokens for a minimum of 90 days may overly expose the Mochi treasury to Curve token price fluctuations.
🌟 Selected for report: leastwood
leastwood
The flashFee() function differs to the implementation found in usdm.flashFee() due to a missing SCALE variable which ensures the result is not truncated. The end result for MochiVault.flashFee() is that users could end up paying a slightly lower fee than intended if _amount is somewhat small.
This could abused by calling flashLoan() with a small _amount input and continually reentering the contract before using the actual funds. This enables a user to pay marginally less in fees as compared to a typical user.
If the asset being held in the vault is configured with a low decimals value, the degree of truncation could be significant, potentially resulting in little to no fees paid back into MochiVault.sol.
Manual code review
Consider updating return (_amount * 1337) / 1000000; in MochiVault.flashLoan() to return (_amount * ((1337 * SCALE) / 1000000)) / SCALE; where SCALE = 1e18.
#0 - r2moon
2021-10-29T10:13:13Z
return (_amount * 1337) / 1000000; and return (_amount * ((1337 * SCALE) / 1000000)) / SCALE; could return same amount i think.
#1 - ghoul-sol
2021-11-02T16:44:05Z
I see a risk of rounding errors but I think this is low risk since small amount flash loans are inpractical