QA Report #155
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Comments
code423n4
added
bug
Proposal Id repetitionValid Low Title: Unlocked pragmaNC Lack of Input ValidationValid Low Approve return valueDisputed as implementation is known, and both tokens always return true (and revert on error) https://etherscan.io/address/0x6b175474e89094c44da98b954eedeac495271d0f#code#L156 Title: Missing Checks For transfer()’s Return statusDisputing per the reason above, and ignoring the ETH one not all ERC20 contracts define decimals() since it’s optional in the specValid low Disagree with rest I feel adding some personality would help a lot as this feels like a scrape of old reports. Also recommend fixing formatting, the title, which should be a heading, is the only line which is not a heading |
|
3L 1NC |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Title: Insufficient Input Validation
Impact
Existing Id could cause AddProposal function to revert.
Proof of Concept
Check if the proposal ID already exists or not.
https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L42
Tools Used
Manual
Recommended Mitigation Steps
Check if the id exists already or not.
Check if the proposal was created successfully.
Title: Unlocked pragma
Impact
Contracts should be deployed using the same compiler version/flags with which they have been tested.
Proof of Concept
see here:
https://swcregistry.io/docs/SWC-103
pragma solidity ^0.8.10
https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L3
…
Tools Used
Manual
Recommended Mitigation Steps
Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.10) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.
Title: Insufficient Input Validation
Impact
The functions should first check if the passed arguments are valid first.
Proof of Concept
External functions that does not check the input values:
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/BaseJumpRateModelV2.sol#L67
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L53
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L64
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L75
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L85
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L95
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L106
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L119
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L140
Tools Used
Manual
Recommended Mitigation Steps
Check input values
Title: Missing Checks For Approve()’s Return status
Impact
Check if this function is set correctly.
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CDaiDelegate.sol#L57
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegate.sol#L41
Tools Used
Manual
Recommended Mitigation Steps
Use require() or require or OpenZeppelin’s safeApprove()
Title: Missing Checks For approve()’s Return status
Impact
Check if this function is set correctly.
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CDaiDelegate.sol#L57
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegate.sol#L41
Tools Used
Manual
Recommended Mitigation Steps
Use require() or require or OpenZeppelin’s safeIncreaseAllowance()/safeDecreaseAllowance()
Title: Missing Checks For transferFrom()’s Return status
Impact
Check if this function transfers assets correctly.
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CDaiDelegate.sol#L57
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegate.sol#L41
Tools Used
Manual
Recommended Mitigation Steps
Use require() for dai
And require or OpenZeppelin’s safeTransferFrom() for note
Title: Missing Checks For transfer()’s Return status
Impact
Check if this function transfers assets correctly.
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L132
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L204
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CEther.sol#L150
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CNote.sol#L131
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ComptrollerG7.sol#L1265
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Maximillion.sol#L43
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Reservoir.sol#L64
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/WETH.sol#L31
Tools Used
Manual
Recommended Mitigation Steps
Use require() or require or OpenZeppelin’s safeTransfer()
Title: Unsafe Casts And Usage Of IERC20Detailed
Impact
not all ERC20 contracts define decimals() since it’s optional in the spec
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L118
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L119
Tools Used
Manual
Recommended Mitigation Steps
Use safeDecimals() instead
decimals0 = 10IERC20Detailed(_token0).decimals();
decimals1 = 10IERC20Detailed(_token1).decimals();
Title: Event is missing indexed fields
Impact
Each event should use three indexed fields if there are three or more fields
Proof of Concept
lending:
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CNote.sol#L10
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Comptroller.sol#L19-L76
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ComptrollerG7.sol#L18-L66
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CTokenInterfaces.sol#L128-L201
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ErrorReporter.sol#L53
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/JumpRateModel.sol#L11
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/NoteInterest.sol#L64-L85
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/SimplePriceOracle.sol#L9
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Timelock.sol#L12-L14
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Unitroller.sol#L16-L31
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/WhitePaperInterestRateModel.sol#L12
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantInterfaces.sol#L24-L26
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Governance/Comp.sol#L48-L57
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Governance/GovernorAlpha.sol#L116-L128
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Governance/GovernorBravoInterfaces.sol#L9-L30
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L100-L104
Tools Used
Manual
Recommended Mitigation Steps
Add Index to at least 3 parameters or existing ones in fewer cases.
Title: Two-step change of privileged roles
Impact
In a single-step change, if the current admin accidentally changes the new admin to a zero-address or an incorrect address (where the private keys are not available), the system is left without an operational admin and will have to be redeployed.
Proof of Concept
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/NoteInterest.sol#L109
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Note.sol#L21
Tools Used
Manual
Recommended Mitigation Steps
When privileged roles are being changed, it is recommended to follow a two-step approach: 1) The current privileged role proposes a new address for the change 2) The newly proposed address then claims the privileged role in a separate transaction. This two-step change allows accidental proposals to be corrected instead of leaving the system operationally with no/malicious privileged role.
The text was updated successfully, but these errors were encountered: