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: 10/15
Findings: 4
Award: $1,094.87
🌟 Selected for report: 3
🚀 Solo Findings: 0
defsec
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call
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); }
Code Review
Consider using safeTransfer/safeTransferFrom or require() consistently.
#0 - r2moon
2021-10-27T13:25:19Z
🌟 Selected for report: nikitastupin
defsec
The getPrice function in the contract ChainlinkAdapter.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices. Stale prices could put funds at risk. According to Chainlink's documentation, This function does not error if no answer has been reached but returns 0, causing an incorrect price fed to the PriceOracle. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations of the AMM. Oracle reliance has historically resulted in crippled on-chain systems, and complications that lead to these outcomes can arise from things as simple as network congestion.
(, int256 price, , , ) = feed[_asset].latestRoundData(); uint256 decimalSum = feed[_asset].decimals() + IERC20Metadata(_asset).decimals();
Code Review
Consider to add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = ETH_CHAINLINK.latestRoundData(); require(price > 0, "Chainlink price <= 0"); require(answeredInRound >= roundID, "..."); require(timeStamp != 0, "...");
#0 - r2moon
2021-10-27T13:19:46Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/87
0.0437 ETH - $182.03
defsec
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/vault/MochiVault.sol#L60 https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/treasury/MochiTreasuryV0.sol#L44
Manual Code Review
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.
#0 - r2moon
2021-10-27T13:30:35Z
i think we don't need to fix them.
🌟 Selected for report: loop
Also found by: WatchPug, defsec, gzeon, harleythedog
defsec
The liquidated variable is redundant. The variable should be deleted for the gas optimization if It is not used.
None
Consider removing the unused function which is not used.
#0 - r2moon
2021-10-27T13:32:27Z
duplicated with https://github.com/code-423n4/2021-10-mochi-findings/issues/88
🌟 Selected for report: defsec
defsec
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
The advantages of versions 0.8.* over <0.8.0 are:
None
Consider to upgrade pragma to at least 0.8.4.
#0 - r2moon
2021-10-27T13:23:47Z
Could you share any links related with that?
#1 - r2moon
2021-10-27T13:24:11Z
Sorry, i clicked wrong button, so closed, but not it has been reopend ;)
🌟 Selected for report: defsec
0.0768 ETH - $319.56
defsec
This does not directly impact the smart contract in anyway besides cost. This is a gas optimization to reduce cost of smart contract. Calling each function, we can see that the public function uses 496 gas, while the external function uses only 261.
According to Slither Analyzer documentation (https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external), there are functions in the contract that are never called. These functions should be declared as external in order to save gas.
Slither Detector:
external-function:
Slither