Platform: Code4rena
Start Date: 30/11/2021
Pot Size: $100,000 USDC
Total HM: 15
Participants: 36
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 62
League: ETH
Rank: 6/36
Findings: 3
Award: $5,427.18
🌟 Selected for report: 5
🚀 Solo Findings: 0
🌟 Selected for report: cyberboy
Also found by: Meta0xNull
3626.1684 USDC - $3,626.17
cyberboy
The __abdicate() function at https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L46-L50 is the logic to remove the governance i.e., to renounce governance. However, the function logic does not consider emergency governor and pending governor, which can be a backdoor as only the "gov" is set to zero address while the emergency and pending gov remains. A pending gov can just claim and become the gov again, replacing the zero address.
Bug 1 3. Call the __abdicate() function and we will notice only "gov" is set to zero address while emergency gov remains.
Bug2 4. Now use the address used in "pendingGov" to call acceptGov() function. 5. We will notice the new gov has been updated to the new address from the zero address.
Hence the __abdicate() functionality can be used as a backdoor using emergency governor or leaving a pending governor to claim later.
Remix to test the poC
The __abdicate() function should set emergency_gov and pendingGov as well to zero address.
#0 - brockelmore
2021-12-06T16:27:51Z
Yes, the governor can be recovered from abdication if pendingGov != 0 as well as emergency gov needs to be set to 0 before abdication because it won't be able to abdicate itself.
Would consider it to be medium risk because chances of it ever being called are slim as it literally would cutoff the protocol from being able to capture its fees.
#1 - 0xean
2022-01-14T22:33:46Z
Given that the functionality and vulnerability exists, and the governor does claim fees, this could lead to the loss of funds. Based on the documentation for C4, that would qualify as high severity.
3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).
🌟 Selected for report: cyberboy
805.8152 USDC - $805.82
cyberboy
Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.
I noticed that almost the entire contract has no zero-address check. I am listing the code and line numbers below.
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol
Line 17 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L17 _governor, _emergency_governor
Line 27 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L27 newPendingGov
Line 41 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L41 who
Line 74 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L74 governor
Line 203 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L203 who
Line 291 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L291-L292 depositLockDuration
Line 300 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L300 rewardToken
Line 432 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L432 Not much impact but an error will lead to tokensNotYetStreamed value of zero address and can lead to incorrect values.
Line 442 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L442 _mint(msg.sender, trueDepositAmt); It may mint to zero address and burn the token
Line 461 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L461
Line 491 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L491
Line 500 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L500 token
Line 516 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L516 token
Line 542 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L542 _burn(msg.sender, amount);
Line 558 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L558
Line 733 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L733 who
Line 780 https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L780 _governor
Note: I have ignored the reports where the address is used without checking for zero address but used in SafeTransferFrom() as it auto handles.
Ideally, the entire contract needs to handle zero address checks, especially for admin functions.
#0 - brockelmore
2021-12-01T08:35:04Z
I appreciate the effort put in here. However, most instances are msg.sender based. Any instance of does not require the check. Likely will keep ability to burn receiptTokens by sending to 0 address as well. There is a case to be made that governance should be required not to start as the 0 address but it is functionally acceptable and a valid state transition via __abdicate anyway. Will review each case and document below
#1 - 0xean
2022-01-16T00:41:16Z
marking down to low risk.
🌟 Selected for report: cyberboy
805.8152 USDC - $805.82
cyberboy
Performing multiplication before division is generally better to avoid loss of precision because Solidity integer division might truncate
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L237-L238 globalStreamingSpeedPerSecond is later used for unstreamed for multiplication after performing division while calculation of globalStreamingSpeedPerSecond
Slither
The code can be optimized to use uint112((uint256(tdelta) * (uint256(unstreamed) * 106) / (endStream - lastUpdate) * 106 Or maybe just (uint112((uint256(tdelta) * (uint256(unstreamed)) / (endStream - lastUpdate)
cyberboy
Note: Please ignore my last submission. It was an error
Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.5.10) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol Is using pragma solidity ^0.8.0;
Also, the imports are using floating pragmas
The correct will be pragma solidity 0.8.0;
And should be made across all the imports.
#0 - 0xean
2022-01-16T13:15:32Z
upgrading to low-risk for this and all the duplicates as this is a best practice for all deployments.
cyberboy
public functions that are never called by the contract should be declared external to save gas. This affects all the Governed contract functions. They have been marked as public but they can be external.
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L316-L318 - tokenAmounts() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L323-L325 - feeParams() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L330-L337 - streamParams() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L365-L368 - getEarned() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L417-L447 - stake() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L487-L494 - exit() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L500-L511 - createIncentive() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L516-L526 - claimIncentive() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L532-L550 - claimDepositTokens() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L555-L578 - claimReward() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L583-L600 - creatorClaimSoldTokens() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L606-L630 - claimFees() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L646-L691 - recoverTokens() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L696-L724 - flashloan() https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L733-L750 - arbitraryCall()
Slither
Mark the following function external instead of the public to save gas.
#0 - 0xean
2022-01-16T14:25:49Z
dupe of #260
🌟 Selected for report: cyberboy
cyberboy
Structs are packed and arrangement matters for packing which in turn affects the gas used.
At line https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L100-L107 If the structs are arranged as below it will consume less gas
struct TokenStream { bool merkleAccess; uint32 lastUpdate; uint112 rewards; uint112 tokens; uint256 lastCumulativeRewardPerToken; uint256 virtualBalance; }
gas used - 166189 instead of 166225 if the existing arrangement is used.
Note: The gas number was calculated only using structs arrangement in self-made contract with the same structs
It is always advised to arrange structs in increasing order to save more gas and not at all mix-up variables sizes.
Truffle
Use the arrangements of structs as mentioned below instead of the existing one
struct TokenStream { bool merkleAccess; uint32 lastUpdate; uint112 rewards; uint112 tokens; uint256 lastCumulativeRewardPerToken; uint256 virtualBalance; }