Open code423n4 opened 2 years ago
NC
Disagree with adding allowance but agree with using safeApprove to avoid nasty compatibility L
L
I don't believe this is the case unless you can demonstrate non-idempotency caused by the order of commands, however in that case the finding would be of higher severity
NC
Disagree because it's mostly opinion based and slither will produce false positives
NC
R
Pretty good
2L 1R 3NC
Table of Contents
abi.encodePacked()
should not be used with dynamic types when passing the result to a hash function such askeccak256()
bytes.concat()
public
functions not called by the contract should be declaredexternal
instead1. Low Risk Issues
1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug
The protocol is using low level calls with solidity version <= 0.8.14 which can result in optimizer bug.
https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d
Consider upgrading to solidity 0.8.15 here:
1.2. Deprecated approve() function
Consider using
safeApprove
instead (or better:safeIncreaseAllowance()
/safeDecreaseAllowance()
):1.3. Missing address(0) checks
Consider adding an
address(0)
check for immutable variables:receiverImplementation
gatewayAddress
This is already done here:
gateway
and here:
AUTH_MODULE
andTOKEN_DEPLOYER_IMPLEMENTATION
1.4.
abi.encodePacked()
should not be used with dynamic types when passing the result to a hash function such askeccak256()
Similar issue in the past: here Original issue: Hash collisions when using abi.encodePacked() with multiple variable length arguments
Use
abi.encode()
instead which will pad items to 32 bytes, which will prevent hash collisions (e.g.abi.encodePacked(0x123,0x456)
=>0x123456
=>abi.encodePacked(0x1,0x23456)
, butabi.encode(0x123,0x456)
=>0x0...1230...456
). If there is only one argument toabi.encodePacked()
it can often be cast tobytes()
orbytes32()
instead.1.5. Use a 2-step ownership transfer pattern
Contracts inheriting from OpenZeppelin's libraries have the default
transferOwnership()
function (a one-step process). It's possible that theonlyOwner
role mistakenly transfers ownership to a wrong address, resulting in a loss of theonlyOwner
role. Consider overriding the defaulttransferOwnership()
function to first nominate an address as thependingOwner
and implementing anacceptOwnership()
function which is called by thependingOwner
to confirm the transfer.2. Non-Critical Issues
2.1. It's better to emit after all processing is done
The
emit
keyword is at line L224, consider moving it after L234:2.2. Use
bytes.concat()
Solidity version 0.8.4 introduces
bytes.concat()
(vsabi.encodePacked(<bytes>,<bytes>)
), this is relevant. Solidity version 0.8.12 introducesstring.concat()
(vsabi.encodePacked(<str>,<str>)
), but it isn't used in the solution, so the upgrade isn't relevant (0.8.9 is ok)2.3.
public
functions not called by the contract should be declaredexternal
instead