GalloDaSballo
commented
2 years ago [L-01] IMMUTABLE ADDRESSES LACK ZERO-ADDRESS CHECK
L
[L-06] execute() FROM Treasury.sol DOESN'T CHECK FOR DYNAMIC INPUT ARRAY LENGTH
R (graceful error)
[L-07] CONSIDER ADDINGS CHECKS FOR SIGNATURE MALLEABILITY
L
[L-08] LACK OF UINT VARIIABLE CHECK
L
[L-09] OWNER CAN INCREMENT BIDDING PRICE ANYTIME
Not upgrading as I believe the report doesn't capture the risk other mentioned
Not mentioned I disagree with
3L 1R
[L-01] IMMUTABLE ADDRESSES LACK ZERO-ADDRESS CHECK
Constructors should check the address written in an immutable address variable is not the zero address
There is 10 instance of this issue:
Mitigation
check for zero address
[L-02] ADDRESSES LACK ZERO-ADDRESS CHECK BEFORE ASSIGNING THEM TO STATE VARIABLE
Should check the addresses before assigning them to state variables
There is 2 instance of this issue:
Mitigation
check for zero address
[L-03] _addFounders() IN Token.sol MAY LEAD TO DoS(block gas limit exceed) CONDITION
_addFounder() used Nested For loops on Dynamic Array inside which forther internal function calls occur which makes state change, This is quite gas consuming, if length of Dynamic Array will too long, then it leads to Block gas limit exceed condition.
There is 1 instance of this issue:
Mitigation
. Use Small size array as Input . Make a Boundary for input array length
[L-04] _SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
There is 1 instance of this issue:
Mitigation
Use SafeMint function.
[L-05] _authorizeUpgrade() OF Manager.sol SHOULD DO SOMETHING, ATLEAST EMIT SOME EVENTS WHEN NEW IMPLEMENTED ADDRESS CHANGE
_authorizeUpgrade() function is just blank block which doing nothing, At least it should emit a events when Implemented address changes
[L-06] execute() FROM Treasury.sol DOESN'T CHECK FOR DYNAMIC INPUT ARRAY LENGTH
3 dynamic array are take as input parameter and then used in a For loop, There is no code syntax for checking 3 arrays are of same length or not. As a Human error Owner can copy paste array of different length that ultimately result in function fail.
Mitigation
Should have condition to check that all arrays have same length.
[L-07] CONSIDER ADDINGS CHECKS FOR SIGNATURE MALLEABILITY
Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly
There is 1 instance of this issue:
[L-08] LACK OF UINT VARIIABLE CHECK
uint check absent for _reservePrice, during initialization in Auction contract, which can be set to 0 at the time of initialization
[L-09] CRITICAL SETTINGS CHANGED BY OWNER ANYTIME
In Auction contract critical settings (Important variables regarding auction) can be changed by owner anytime using functions like setDuration(), setReservePrice(), setTimeBuffer(), setMinimumBidIncrement()
Mitigation
There should be some governace system with a Timelock feature, that gives Audience to make their decisions
[L-09] OWNER CAN INCREMENT BIDDING PRICE ANYTIME
No check presents for input parameter percentage in function setMinimumBidIncrement(), that could be Any orbitary amount. This could lead to a Front-Running case,
Mitigation
There should be a Timelock feature for changing critical feature like this, that gives Audience to make their decisions
[L-10] NO CHECK FOR RETURN VALUE OF IWETH TRANSFER
Their is no check for return value of IWETH.transfer that could lead to the Eth loss for bidder in Auction contract.
Mitigation
There should be a return value check present for IWETH.transfer More important here Contract using Pushing instaed of Pulling which can lead to DoS if attacker intend to so, and he will lose his fund