User may charge any amount of fees when registering a subprotocol

Lines of code Vulnerability details Impact When registering a subprotocol, the user may enter any amount of fees. There are no checks implemented. Proof of Concept The register function does not have a check for the fee. A limit may be imposed otherwise a user can register a subprotocol with any...

Misuse of a Boolean constant

Lines of code Vulnerability details Impact Use of Boolean constants true/false in code is indicative of flawed logic. Boolean constants in code have only a few legitimate uses. Other uses in complex expressions, as conditionals indicate either an error or, most likely, the persistence of faulty...

UNEXPECTED DATA LOSS IS POSSIBLE DUE TO UNSAFE DOWN CASTING OF UINT40 TO UINT32

Lines of code Vulnerability details Impact In the dripsRange function of the Drips.sol contract variable end is declared as of type uint40. After the arithmetic operations and conditional checks have been performed, the end variable is returned as uint32end thus down casting it unsafely. Proof of...

A Theoretical-Gaming Vulnerability

Lines of code Vulnerability details Impact There is a theoretical-gaming vulnerability in the project. Currently, users are able to set up a graph in which money should flow from one vertex to another. The user sets the split configuration on their account and there is a public function...

Upgraded Q -> 3 from #508 [1675443043181]

Judge has assessed an item in Issue 508 as 3 risk. The relevant finding follows: L-05 Duration does not have upper bound The duration input parameter does not have upper bound. If the duration is mistakenly set too high, node operator will be slashed significant amount of GGP. The...

If no association type matched, user will end up paying fee for nothing

Lines of code Vulnerability details Impact Users can add a new entry for the given subprotocol to the provided CID NFT. There are possible three different association types ordered, primary, active that can be used to model different types of associations between the CID NFT and subprotocol. For...

Upgraded Q -> 2 from #615 [1675442567749]

Judge has assessed an item in Issue 615 as 2 risk. The relevant finding follows: 3:upgradeExistingContract need unregisterContract first and then registerContract. Avoid newAddr==existingAddr. unregisterContract remove newAddr function upgradeExistingContract address newAddr, string memory newNam...

Drips that end after the current cycle but before its creation can allow users to profit from squeezing

Lines of code Vulnerability details Impact By creating a drip that ends after the current cycle but before its creation time and immediately removing it, the sender doesn't have to put in any assets but the receiver can still squeeze this drip. By setting a receiver that the sender controls, the...

Upgraded Q -> 2 from #260 [1675460942583]

Judge has assessed an item in Issue 260 as 2 risk. The relevant finding follows: L2 Withdrawable minipool can be finished before funds are withdrawn --- The text was updated successfully, but these errors were encountered: All reactions...

Unsafe casting of an int128 to uint128 can cause wrong accounting in _receiveDripsResult

Lines of code Vulnerability details Impact Unsafe casting of int128 to uint128 is dangerous and makes user receive more assets than they should. Proof of Concept state.amtDeltascycle can be a negative value. The delta value at each point is simply the relative change from the previous cycle. If...

Upgraded Q -> 2 from #508 [1675443058277]

Judge has assessed an item in Issue 508 as 2 risk. The relevant finding follows: New address and existing address inputs can be the same in upgradeExistingContract --- The text was updated successfully, but these errors were encountered: All reactions...

Incorrect shift in assembly

Lines of code Vulnerability details Impact Shift operators shlx, y, shrx, y, sarx, y in Solidity assembly apply the shift operation of x bits on y and not the other way around, which may be confusing. Check if the values in a shift operation are reversed. Proof of Concept File: Drips.sol 823: val...

Some ERC20 tokens deduct a fee on transfer

Lines of code Vulnerability details Impact Some ERC20 token implementations have a fee that is charged on each token transfer. This means that the transferred amount isn't exactly what the receiver will get. A call to ERC20token.transferrecipient, 100 with a fee-on-transfer of 5% will entitle the...

Upgraded Q -> 2 from #846 [1675451731129]

Judge has assessed an item in Issue 846 as 2 risk. The relevant finding follows: L-2 no way to remove compromised/broken multisigs without upgrading the contract --- The text was updated successfully, but these errors were encountered: All reactions...

User can do registration of one CID NFT many times

Lines of code Vulnerability details Impact User can do registration one CID NFT repeatedly, and event CIDNFTAdded will be emited again and again. No checks, that user already registerd. When willl be some logic on frontend, which use this event, it could break, when get one more events CIDNFTAdde...

Upgraded Q -> 2 from #748 [1675439377792]

Judge has assessed an item in Issue 748 as 2 risk. The relevant finding follows: L MultisigManager doesn't have unregister function MultisigManager.sol contains a hard limit of MULTISIGLIMIT to 10. Meanwhile in the contract, there are functions like registerMultisig, enableMultisig, and...

Upgraded Q -> 2 from #769 [1675429128999]

Judge has assessed an item in Issue 769 as 2 risk. The relevant finding follows: L-1 requireNextActiveMultisig always returns the 1st enabled Multisig Relevant code: As the name suggested, MultisigManager.requireNextActiveMultisig should return the next enabled Multisig. However, it actually alwa...

nftAddress can be a malicious NFT

Lines of code Vulnerability details Impact In the subprotocolregistry contract, users are allowed to register any subprotocols. As long as the supportsinterface condition is met. The validity of the nftaddress address is not checked. function register bool ordered, bool primary, bool active,...

_squeezeDrips() passed the amount argument in place of amtPerSec for the _addDeltaRange, causing either underflow or the sender losing lots of fund!

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. squeezeDrips passes the amount argument in place of amtPerSec for its callee addDeltaRange in the following line addDeltaRangestate, cycleStart, cycleStart + 1, -int256amt AMTPERSECMULTIPLIER; The last...

Solmate safeTransfer and safeTansferFrom does not check the code size of the token address

Lines of code Vulnerability details Impact The safeTransfer and safeTransferFrom don't check the existence of code at the token address. This is a known issue while using solmate's libraries. Hence this may lead to miscalculation of funds and may lead to loss of funds, because if safeTransfer and...

Multiple accounts can have the same identity

Lines of code Vulnerability details Users can register their on-chain identity ie their CID NFT by calling AddressRegistry.register File: src/AddressRegistry.sol 42: function registeruint256 cidNFTID external 43: if ERC721cidNFT.ownerOfcidNFTID != msg.sender 44: // We only guarantee that a CID NF...

Upgraded Q -> 2 from #65 [1675444463774]

Judge has assessed an item in Issue 65 as 2 risk. The relevant finding follows: Unusual multisig logic --- The text was updated successfully, but these errors were encountered: All reactions...

MinipoolManager: node operator can avoid being slashed

Lines of code Vulnerability details Impact When staking is done, a Rialto multisig calls MinipoolManager.recordStakingEnd . If the avaxTotalRewardAmt has the value zero, the MinipoolManager will slash the node operator's GGP. The issue is that the amount to slash can be greater than the GGP balan...

Lack of double step transfer in admin modification in a upgradeable contract is dangerous

Lines of code Vulnerability details Lack of double step transfer in admin modification in a upgradeable contract is dangerous Summary Double step transfer of admin / ownership should be a must in upgradeable contracts Vulnerability Detail Admin is changed with changeAdmin that calls changeAdmin,...

Same identity can be assigned to multiple users

Lines of code Vulnerability details Impact In the contest details it is mentioned that identity can be transferable even if it's currently assigned to address in AddressRegistry. However, I would assume that if another address registers it, the identity should be removed from the previous owner...

User can lose NFT if wrong type is given to add function

Lines of code Vulnerability details Impact If a user when trying to add its NFT to a given subprotocol provide a wrong association type by accident to the add function, the NFT will be transferred to the CidNFT contract but it will not be associated with any protocol type, because of that when th...

Split vulnerable to preimage attack

Lines of code Vulnerability details Impact A motivated attacker could invest the resources to craft a malicious SplitsReceiver to steal all of a users' pending funds. Proof of Concept This is a non-practical implementation of the attack, but shows by extending the SplitsReceiver array by any numb...

Potential DOS in Contract Inheriting UUPSUpgradeable.sol

Lines of code Vulnerability details Impact There is a contract which inherit UUPSUpgradeable.sol, namely; Managed.sol . The contract is deployed using a proxy pattern whereby the implementation contract is used by the proxy contract for all its logic. The proxy contract will make delegate calls t...

_receiveDripsResult() overcounts amtPerCycle

Lines of code Vulnerability details Impact Drips results will be accounted for wrongly and hence users will receive more drips than they should. Proof of Concept According to the whitepaper, amtDeltas stored at each cycle is the value relative to the previous cycle. The stored delta for a cycle i...

Upgraded Q -> 2 from #783 [1675429188655]

Judge has assessed an item in Issue 783 as 2 risk. The relevant finding follows: restakeGGP doesn't have whenNotPaused modifier At line 319 stakeGGP has whenNotPaused modifier, while restakeGGP has not. Suggest to add the same modifier to restakeGGP. --- The text was updated successfully, but the...

AddressRegistry can associate same CID to different addresses at the same time

Lines of code Vulnerability details The AddressRegistry contract can associate a CID NFT to an account address. As stated in the contest, the CID NFT can be transferred out of the account that registered it. However, once transferred it can be registered again while keeping the previous...

Anyone can call the DripsHub.split function to frontrun the DripsHub.setSplits function call of the trusted user who owns the splittable funds

Lines of code Vulnerability details Impact As mentioned by the following DripsHub.split function's comment, the user, who owns the splittable funds, needs to be trusted by the receivers for such splittable funds. Yet, when this trusted user wants to call the DripsHub.setSplits function below to...

Upgraded Q -> 2 from #365 [1675443623313]

Judge has assessed an item in Issue 365 as 2 risk. The relevant finding follows: L-4 Misleading comments - Multisig are still managing pool --- The text was updated successfully, but these errors were encountered: All reactions...

CidNFT#add will break if _type is not passed in correctly

Lines of code Vulnerability details Impact CIDNFT holders that wants to add subprotocols to their NFT will not be able to anymore because the subprotocol NFT will be stuck in the CidNFT.sol contract. Proof of Concept When a CIDNFT holder wants to add subprotocols to their NFT, he will call...

int128 cast underflow in _receiveDripsResult()

Lines of code Vulnerability details Impact In receiveDripsResult, the type cast of uint128 could underflow, and result in wrong receivedAmt. The impacts could be: wrong amount being transferred to users and drain the protocol fund inaccurate transfer amount, some users lose fund and some receive...

_updateReceiverStates CAN WORK INCORRECTLY WHEN CURRENT RECEIVER AND NEW RECEIVER MATCHES WITH UPDATED DRIP TIMES

Lines of code Vulnerability details Impact States can be incorrectly updated. Proof of Concept In updateReceiverStates method of Drips.sol, I noticed a different pattern of Using addDeltaRange when current receiver and new receiver matches with updated drip times. To Remove an existing drip, the...

Upgraded Q -> 2 from #338 [1675444014859]

Judge has assessed an item in Issue 338 as 2 risk. The relevant finding follows: L-07 It should be possible to assign Minipool to a new Multisig MinipoolManager.sol 1 --- The text was updated successfully, but these errors were encountered: All reactions...

Upgraded Q -> 2 from #854 [1675461747744]

Judge has assessed an item in Issue 854 as 2 risk. The relevant finding follows: When the protocol is paused, all the multisigs are disabled:, However, it is still possible to call startRewardsCycle in the RewardsPool, however, the execution will revert because the enabled count is 0: --- The tex...

Admin role lockout possible

Lines of code Vulnerability details Impact Admin of contracts is controlled by Managed contract, and implements a transfer of admin privilege in a single step. A malicious admin or an error in the new address when calling changeAdmin can prevent all admin activities on all the contracts forever...

Upgraded Q -> 3 from #867 [1675460716325]

Judge has assessed an item in Issue 867 as 3 risk. The relevant finding follows: L-02, MinipoolManager, lines 670 - 684: The slash function slashes a node operator for the amount of whole duration. Since the cycles are in 14 days and the slashing is checked in the recordStakingEnd, if an operator...

Signature replay attacks possible if deployed on multiple chains

Lines of code Vulnerability details Caller.callSigned operates using an EIP-712 signature which verifies the signed data to be used in a call on behalf of the signer. The problem with this method lies in the fact that it doesn't specify the chain ID, and thus if the contract is ever deployed to...

Support for fee-on-transfer tokens

Lines of code Vulnerability details Impact The AddressDriver.sol do not support fee-on-transfer tokens. If the asset is a fee-on-transfer token, tokens received from users could be less then the amount specified in the transfer. The protocol could suffer a loss of funds. Proof of Concept function...

Code breaks if first user is not expected user

Lines of code Vulnerability details Code breaks if first user is not expected user Summary Rather than iterate and continue if user is not the expected one, this code breaks all the execution if first user is userId Vulnerability Detail Execution is broke most of the times at first iteration for ...

## Mathematical Modelling Cause a Significant Roundoff Error

Lines of code Vulnerability details Mathematical Modelling Cause a Significant Roundoff Error Function drippedAmt calculates the ended cycles. when calculating ended cycles it takes two divisions which cause round off error. Let's consider this scenario. Deposited token - WBTC amtPerSec = 1000000...

Upgraded Q -> 2 from #800 [1675429611496]

Judge has assessed an item in Issue 800 as 2 risk. The relevant finding follows: 1- recordStakingError function doesn't decrease the minipool avaxLiquidStakerAmt value : When the function recordStakingError is called by the multisig it decreases both the total AVAX staking amount and the AVAX...

Caller.callBatched doesn't enforce msg.value is equal to sum of call values

Lines of code Vulnerability details For each call in callBatched, we pass a value to be sent along with the call: for uint256 i = 0; i calls.length; i++ Call memory call = callsi; returnDatai = callsender, call.to, call.data, call.value; The sum of the values of the calls should be equal to...

Reentrancy due to lack of check-effect-interact pattern by valid Driver users

Lines of code Vulnerability details Impact In a Re-entrancy attack, a malicious contract calls back into the calling contract before the first invocation of the function is finished. This may cause the different invocations of the function to interact in undesirable ways, especially in cases wher...

Upgraded Q -> 2 from #867 [1675460709593]

Judge has assessed an item in Issue 867 as 2 risk. The relevant finding follows: L-01, ProtocolDAO.sol lines 209 - 216: upgradeExistingContract mistakenly removes the address value of the new contract if the new contract’s name is the same as the old one. This can be easily fixed with unregisteri...

DIVISION BY _AMT_PER_SEC_MULTIPLIER AT EACH STEP OF THE ARITHMETIC OPERATION BEFORE MULTIPLICATION RESULTS IN ROUNDING ERROR

Lines of code Vulnerability details Impact In the drippedAmt function of the Drips.sol contract, the assembly is used to calculate the amount dripped over a time range. Inside the assembly amtPerCycle is calculated by multiplying cycleSecs and amtPerSec and dividing by AMTPERSECMULTIPLIER to remo...

Hacked or malicious owner can steal all tokens

Lines of code Vulnerability details Tokens for all active drips are stored in the DripsHub contract. Since DripsHub is an upgradeable ERC1967Proxy, a malicious or hacked owner can simply upgrade the contract to include e.g., the following function: function stealTokensIERC20 token, address to,...