 |
Prepared by: 0xVolodya, Independent Security Researcher Date: July 25 to Aug 1, 2023 |
An open lending and borrowing DeFi protocol built on Base, Moonbeam, and Moonriver.
| ID | Title | Severity | Fixed |
|---|---|---|---|
| [M-01] | One check is missing when proposal execution bypassing queuing | Medium | ✓ |
| [M-02] | Its not possible to liquidate deprecated market | Medium | ✓ |
| [M-03] | Unsafe use of transfer()/transferFrom() with IERC20 | Medium | ✓ |
| [M-04] | Insufficient oracle validation | Medium | ✓ |
| [M-05] | Missing checks for whether the L2 Sequencer is active | Medium | ✓ |
| [M-06] | The owner is a single point of failure and a centralization risk |
Medium | ✓ |
| [M-07] | Some tokens may revert when zero value transfers are made | Medium | ✓ |
Detailed description of the impact of this finding. One check is missing when proposal execution bypassing queuing
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
In TemporalGovernor, executeProposal can be executed with a flag bypassing queuing, so there are the same checks as inside queueProposal, but there is one check that missing, and according to comments is important:
// Very important to check to make sure that the VAA we're processing is specifically designed
// to be sent to this contract
require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");Governance/TemporalGovernor.sol#L311
so, the same check should be added to executeProposal
address[] memory targets; /// contracts to call
uint256[] memory values; /// native token amount to send
bytes[] memory calldatas; /// calldata to send
- (, targets, values, calldatas) = abi.decode(
+ (intendedRecipient, targets, values, calldatas) = abi.decode(
vm.payload,
(address, address[], uint256[], bytes[])
);
+ require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");
/// Interaction (s)
_sanityCheckPayload(targets, values, calldatas);
Detailed description of the impact of this finding. Its not possible to liquidate deprecated market
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
Currently in the code that is a function _setBorrowPaused that paused pause borrowing. In origin compound code borrowGuardianPaused is used to do liquidate markets that are bad. So now there is not way to get rid of bad markets.
function liquidateBorrowAllowed(
address mTokenBorrowed,
address mTokenCollateral,
address liquidator,
address borrower,
uint repayAmount) override external view returns (uint) {
// Shh - currently unused
liquidator;
if (!markets[mTokenBorrowed].isListed || !markets[mTokenCollateral].isListed) {
return uint(Error.MARKET_NOT_LISTED);
}
/* The borrower must have shortfall in order to be liquidatable */
(Error err, , uint shortfall) = getAccountLiquidityInternal(borrower);
if (err != Error.NO_ERROR) {
return uint(err);
}
if (shortfall == 0) {
return uint(Error.INSUFFICIENT_SHORTFALL);
}
/* The liquidator may not repay more than what is allowed by the closeFactor */
uint borrowBalance = MToken(mTokenBorrowed).borrowBalanceStored(borrower);
uint maxClose = mul_ScalarTruncate(Exp({mantissa: closeFactorMantissa}), borrowBalance);
if (repayAmount > maxClose) {
return uint(Error.TOO_MUCH_REPAY);
}
return uint(Error.NO_ERROR);
}
I think compound have a way to liquidate deprecated markets for a safety reason, so it needs to be restored
function liquidateBorrowAllowed(
address mTokenBorrowed,
address mTokenCollateral,
address liquidator,
address borrower,
uint repayAmount) override external view returns (uint) {
// Shh - currently unused
liquidator;
/* allow accounts to be liquidated if the market is deprecated */
+ if (isDeprecated(CToken(cTokenBorrowed))) {
+ require(borrowBalance >= repayAmount, "Can not repay more than the total borrow");
+ return uint(Error.NO_ERROR);
+ }
if (!markets[mTokenBorrowed].isListed || !markets[mTokenCollateral].isListed) {
return uint(Error.MARKET_NOT_LISTED);
}
/* The borrower must have shortfall in order to be liquidatable */
(Error err, , uint shortfall) = getAccountLiquidityInternal(borrower);
if (err != Error.NO_ERROR) {
return uint(err);
}
if (shortfall == 0) {
return uint(Error.INSUFFICIENT_SHORTFALL);
}
/* The liquidator may not repay more than what is allowed by the closeFactor */
uint borrowBalance = MToken(mTokenBorrowed).borrowBalanceStored(borrower);
uint maxClose = mul_ScalarTruncate(Exp({mantissa: closeFactorMantissa}), borrowBalance);
if (repayAmount > maxClose) {
return uint(Error.TOO_MUCH_REPAY);
}
return uint(Error.NO_ERROR);
}
+ function isDeprecated(CToken cToken) public view returns (bool) {
+ return
+ markets[address(cToken)].collateralFactorMantissa == 0 &&
+ borrowGuardianPaused[address(cToken)] == true &&
+ cToken.reserveFactorMantissa() == 1e18
+ ;
+ }
Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert (see this link for a test case). Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
There are 2 instances of this issue:
File: src/core/Comptroller.sol
965: token.transfer(admin, token.balanceOf(address(this)));
967: token.transfer(admin, _amount);
There is no freshness check on the timestamp of the prices fetched from the Chainlink oracle, so old prices may be used if OCR was unable to push an update in time. Add a staleness threshold number of seconds configuration parameter, and ensure that the price fetched is from within that time range.
There are 2 instances of this issue:
File: src/core/Oracles/ChainlinkCompositeOracle.sol
183 (
184 uint80 roundId,
185 int256 price,
186 ,
187 ,
188 uint80 answeredInRound
189: ) = AggregatorV3Interface(oracleAddress).latestRoundData();
File: src/core/Oracles/ChainlinkOracle.sol
100 (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
101: .latestRoundData();
Chainlink recommends that users using price oracles, check whether the Arbitrum Sequencer is active. If the sequencer goes down, the Chainlink oracles will have stale prices from before the downtime, until a new L2 OCR transaction goes through. Users who submit their transactions via the L1 Dealyed Inbox will be able to take advantage of these stale prices. Use a Chainlink oracle to determine whether the sequencer is offline or not, and don't allow operations to take place while the sequencer is offline.
There are 2 instances of this issue:
File: src/core/Oracles/ChainlinkCompositeOracle.sol
189: ) = AggregatorV3Interface(oracleAddress).latestRoundData();
File: src/core/Oracles/ChainlinkOracle.sol
100 (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
101: .latestRoundData();
Having a single EOA as the only owner of contracts is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Consider changing to a multi-signature setup, or having a role-based authorization model.
There are 2 instances of this issue:
File: src/core/Governance/TemporalGovernor.sol
266: function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
274: function togglePause() external onlyOwner {
In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.
There are 2 instances of this issue:
File: src/core/Comptroller.sol
964 if (_amount == type(uint).max) {
965 token.transfer(admin, token.balanceOf(address(this)));
966 } else {
967: token.transfer(admin, _amount);
962 IERC20 token = IERC20(_tokenAddress);
963 // Similar to mTokens, if this is uint.max that means "transfer everything"
964 if (_amount == type(uint).max) {
965: token.transfer(admin, token.balanceOf(address(this)));