Skip to content

Instantly share code, notes, and snippets.

@cylon56

cylon56/compound-pr193-pr177-audit.md Secret

Last active June 2, 2022 22:34
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save cylon56/752f9061713a8d737e526fdce4b85f1f to your computer and use it in GitHub Desktop.
Save cylon56/752f9061713a8d737e526fdce4b85f1f to your computer and use it in GitHub Desktop.
Download ZIP
Raw
compound-pr193-pr177-audit.md

Audit of Compound Changes

Performed by OpenZeppelin

Date: 5-11-22

For PR193:

  • M01: The sweep function misses to check that admin is not sweeping COMP token.
  • L01: EIP20NonStandardInterface.transfer not checked to silently fail inside. Balance before and after could be checked to avoid silent fails in the internal execution of the transfer call. The standard EIP20 interface can be used if the goal is to only sweep WBTC.
  • L02: ErrorReporter and the ComptrollerInterface are being imported but the contract is not extending from them.
  • N01: Missing error string in require statement in line 20
  • N02: The contract is not using the named import syntax to explicitly declare which contracts are being imported.
  • N03: Solidity version is not pinned and is outdated.
  • N04: Compound is used to add "_" prefix for admin functions and even though sweep is an admin function does not have it.
  • N05: Missing docstrings for both sweep and _become functions.
  • N06: Visibility of sweep and _become function should be external if those are not called internally. Note: this audits assume that the proposer MUST be sure to use the right parameters when creating the proposal.

For PR177:

  • L01: Someone can still send ethers to the Governor by using selfdestruct. This change makes impossible to bring them out if this happens. Remediation of this issue is to refactor the code to accept eth through a receive payable function and to send those to the Timelock before proposal execution. Similar (but not exactly the same) to what OZ does.
@cylon56
Copy link
Author

cylon56 commented Jun 2, 2022

Update for PR193: All issues have been addressed with fixes in PR#6 except for the outdated Solidity version, mantained to be aligned with the rest of the contracts as mentioned by the developer. Notice that this pull request is of a branch on top of the proposed changes in the original PR#193. We assume it will be merged as it is with no further changes as per commit 75d98843dea3bbb8d935cf786547c36fcb01b6cf.

Update for PR177: The issue is not going to be fixed. In the words of the developer: "The theoretical ability to brick eth in the contract through intentional manipulation is not an issue for us".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment