Platform: Code4rena
Start Date: 15/12/2022
Pot Size: $128,000 USDC
Total HM: 28
Participants: 111
Period: 19 days
Judge: GalloDaSballo
Total Solo HM: 1
Id: 194
League: ETH
Rank: 58/111
Findings: 1
Award: $122.82
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0xSmartContract, AkshaySrivastav, HollaDieWaldfee, RaymondFam, Rolezn, ast3ros, betweenETHlines, brgltd, chaduke, codeislight, cozzetti, lukris02, pauliax, sces60107
122.8177 USDC - $122.82
During the audit, 4 low and 12 non-critical issues were found.
| â„– | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | The recordStakingEnd() function does not match the documentation exactly | Low | 1 |
| L-2 | getMinipools() function may unexpectedly revert | Low | 1 |
| L-3 | getStakers() function may unexpectedly revert | Low | 1 |
| L-4 | "Dirty hack" | Low | 3 |
| NC-1 | Open TODOs | Non-Critical | 3 |
| NC-2 | Order of Functions | Non-Critical | 13 |
| NC-3 | Order of Layout | Non-Critical | 7 |
| NC-4 | Empty function body | Non-Critical | 1 |
| NC-5 | Typos in event names | Non-Critical | 3 |
| NC-6 | Typos in comments | Non-Critical | 22 |
| NC-7 | Missing NatSpec | Non-Critical | 16 |
| NC-8 | Unused named return variables | Non-Critical | 2 |
| NC-9 | Maximum line length exceeded | Non-Critical | 25 |
| NC-10 | Use Checks Effects Interactions pattern | Non-Critical | 1 |
| NC-11 | Code can be made more consistent | Non-Critical | 1 |
| NC-12 | Move the function for clarity | Non-Critical | 1 |
recordStakingEnd() function does not match the documentation exactlyDocs says:
"Staking rewards are split between Node Operators and Liquid Stakers with Node Operators getting 50% + a variable commission fee, and Liquid Stakers receiving the remainder."
But the Lines 417-418 are:
uint256 avaxLiquidStakerRewardAmt = avaxHalfRewards - avaxHalfRewards.mulWadDown(dao.getMinipoolNodeCommissionFeePct()); uint256 avaxNodeOpRewardAmt = avaxTotalRewardAmt - avaxLiquidStakerRewardAmt;
Thus, even if the rewards are calculated correctly, the mismatch between code and documentation can be misleading.
Change the documentation to:
"Staking rewards are split between Node Operators and Liquid Stakers with Liquid Stakers getting 50% - a variable commission fee, and Node Operators receiving the remainder."
or
Change the code to:
uint256 avaxNodeOpRewardAmt = avaxHalfRewards + avaxHalfRewards.mulWadDown(dao.getMinipoolNodeCommissionFeePct()); uint256 avaxLiquidStakerRewardAmt = avaxTotalRewardAmt - avaxNodeOpRewardAmt;
getMinipools() function may unexpectedly revertIn function getMinipools(), if parameter offset is bigger than totalMinipools, this line causes revert():
minipools = new Minipool[](max - offset);
It may be not obvious what the cause of the error is, so it is better to handle this case.
Before L617 in MinipoolManager.sol add the check:
if (offset > max) { //or if (offset > totalMinipools) revert OffsetIsBiggerThanMinipoolsAmount(); }
getStakers() function may unexpectedly revertIn function getStakers(), if parameter offset is bigger than totalStakers, this line causes revert():
stakers = new Staker[](max - offset);
It may be not obvious what the cause of the error is, so it is better to handle this case.
Before L426 in Staking.sol add the check:
if (offset > max) { //or if (offset > totalStakers) revert OffsetIsBiggerThanStakersAmount(); }
The code contains a comment "// Dirty hack to cut unused elements off end of return value (from RP)", which gives the impression that something bad is being done and can cause mistrust.
Consider reformulating the comment.
// TODO remove this when dev is complete// TODO Revisit this logic if we ever allow unequal matched funds// TODO cant use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) since we also call from increaseMinipoolCount. Wat do?Resolve issues.
According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:
Public functions should be placed after external:
External functions should be placed before public:
Private functions should be placed at the end of the contract:
Constructor should be placed before all functions:
Internal functions should be placed after public:
Reorder functions where possible.
According to Order of Layout, inside each contract, library or interface, use the following order:
Events should be placed after state variables:
Place events after state variables.
The function has no comments and no code.
Consider adding a comment to clarify the purpose of the function.
event ClaimNodeOpRewardsTransfered(uint256 value); => Transferredevent ProtocolDAORewardsTransfered(uint256 value); => Transferredevent MultisigRewardsTransfered(uint256 value); => Transferred/// @dev Eligiblity: time in protocol (secs) > RewardsEligibilityMinSeconds. Rialto will call this. => Eligibilityminipool.item<index>.avaxTotalRewardAmt = Actual total avax rewards paid by avalanchego to the TSS P-chain addr => avalanche go/// @param txID The ID of the transaction that successfully registered the node with Avalanche to become a validater => validator/// @param avaxTotalRewardAmt The rewards the node recieved from Avalanche for being a validator => received// Calculate rewards splits (these will all be zero if no rewards were recvd) => received/// @notice Re-stake a minipool, compounding all rewards recvd => received/// @return The approximate rewards the node should recieve from Avalanche for beign a validator => receive/// @return The approximate rewards the node should recieve from Avalanche for beign a validator => being/// @return minipools in the protocol that adhear to the paramaters => adhere/// @return minipools in the protocol that adhear to the paramaters => parameters/// @dev Multisigs will need to be enabled seperately, we dont know which ones to enable => separately/// @notice The node commision fee for running the hardware for the minipool => commission/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name => unregistering/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name => registering/// @return stakers in the protocol that adhear to the paramaters => adhere/// @return stakers in the protocol that adhear to the paramaters => parameters// Emit the withdrawl event for that contract => withdrawn/// @dev No funds actually move, just bookeeping => bookkeeping/// @param toContractName Name of the contract fucns are being transferred to => fucns?/// @param withdrawalAddress Address that will recieve the token => receive// Get the toke ERC20 instance => token// The constructor is exectued only when creating implementation contract => executedNatSpec is missing for 16 functions in 2 contracts.
and 14 out of 18 functions in TokenggAVAX.sol
Add NatSpec for all functions.
Both named return variable(s) and return statement are used.
To improve clarity use only named return variables.
For example, change:
function functionName() returns (uint id) { return x;
to
function functionName() returns (uint id) { id = x;
According to Style Guide, maximum suggested line length is 120 characters.
Make the lines shorter.
It is best practice to use this pattern.
Link:
// Send tokens to this address now, safeTransfer will revert if it fails tokenContract.safeTransferFrom(msg.sender, address(this), amount); // Update balances tokenBalances[contractKey] = tokenBalances[contractKey] + amount;
Swap these lines of code
There are two functions that behave similarly, and in a similar part of the code, one uses getUint(keccak256("minipool.count")); , and the other one calls the function getStakerCount();.
function getMinipools:
uint256 totalMinipools = getUint(keccak256("minipool.count"));
function getStakers:
uint256 totalStakers = getStakerCount();
To make code more consistent and clear:
uint256 totalMinipools = getMinipoolCount();
oruint256 totalStakers = getUint(keccak256("staker.count"));For clarity, it is better to place function getMinipoolCount() before function getMinipools().
#0 - GalloDaSballo
2023-01-24T20:24:48Z
L-1 The recordStakingEnd() function does not match the documentation exactly Low 1 L
L-2 getMinipools() function may unexpectedly revert Low 1 R
L-3 getStakers() function may unexpectedly revert Low 1 R
L-4 "Dirty hack" Low 3 Invalid, for lack of proof
NC-1 Open TODOs Non-Critical 3 NC
NC-2 Order of Functions Non-Critical 13 NC
NC-3 Order of Layout Non-Critical 7 NC
NC-4 Empty function body Non-Critical 1 Nc
NC-5 Typos in event names Non-Critical 3 NC
NC-6 Typos in comments Non-Critical 22 NC
NC-7 Missing NatSpec Non-Critical 16 NC
NC-8 Unused named return variables Non-Critical 2 R
NC-9 Maximum line length exceeded Non-Critical 25 NC
NC-10 Use Checks Effects Interactions pattern Non-Critical 1 L
NC-11 Code can be made more consistent Non-Critical 1 R
NC-12 Move the function for clarity NC
#1 - GalloDaSballo
2023-02-03T13:26:53Z
2L 4R 9NC
#2 - c4-judge
2023-02-03T22:09:43Z
GalloDaSballo marked the issue as grade-b