Issues found - Githubissues

sakulstra commented 9 months ago

[ ] WGHO is missing a license specifier
[ ] there's unused imports like import {AaveGovernanceV2} from '@aave/AaveGovernanceV2.sol';, which don't change the code but will make verificated code on etherscan messy.
[ ] the remappings are quite weird, aave-address-book & solidity-utils are bgd, why remapping one to @aave one to @bgd. It's minor, but as this is non upgradable and the metadata will be on etherscan i think better to do:
```
aave-address-book/=lib/aave-address-book/src/
solidity-utils/=lib/solidity-utils/src/
```
[ ] better to consistently use named imports (so no import './interfaces/IGHO.sol'; etc)
[ ] not sure but you might want to add aci as author
[ ] https://github.com/AaveChan/WGHO/blob/master/src/WGHO.sol#L59 would put a check for address(0)
[ ] might make sense to consistently use ECDSA from oz

Tests

JosepBove commented 9 months ago

On it!

kyzia551 commented 9 months ago

ERC20 getting imported twise here and here
errors should be moved to the interface
PermitParams should not be a part of METADEPOSIT_TYPEHASH and METAWITHDRAWAL_TYPEHASH
If Permit will be overtaken by malicious actor and executed in advance tx will fail. This issue was known time ago, use try -> catch around instead
does not look reasonable to do checks of allowance and balance here, because it will fail anyway on safeTransfer
should be used _hashTypedDataV4 here instead example is here
here should be amount > balanceOf - totalSupply
on deadline and owner check use the same errors as here OZ uses
here and here use internal _useNonce function same as OZ doing it here
nonces should not be overloaded, because of the previous point
internal _withdraw should be defined, and reused in regular one and meta

AaveChan / WGHO