Platform: Code4rena
Start Date: 15/07/2021
Pot Size: $80,000 USDC
Total HM: 28
Participants: 18
Period: 7 days
Judge: ghoulsol
Total Solo HM: 18
Id: 20
League: ETH
Rank: 3/18
Findings: 9
Award: $8,698.83
🌟 Selected for report: 14
🚀 Solo Findings: 0
0xRajeev
decreaseAllowance or approve() to lower the allowance will fail silently, i.e. not work as expected, if previous approval has set allowance to type(uint256).max. While unsafe, it is common to request/give max approval to contracts. However, in case of contract vulnerabilities or attempted rug-pulls, it is also common for users to decrease the approval to 0.
The current logic prevents the above decreaseAllowance/approval flow because it does not allow resetting allowance if current allowance is type(uint256).max. It assumes that approval is always trying to increase allowance (as noted in the code comment “/ No need to re-approve if already max”) and there is nothing to do given current allowance is already uint256.max. This incorrect conditional optimization puts user funds at risk for all Pools, Synths and SPARTA tokens.
Typical _approve(): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3935b907d40c9a23b04b721c2f61758df1caf722/contracts/token/ERC20/ERC20.sol#L303-L313
Manual Analysis
Remove conditional optimization to allow reduction of allowance even if it is currently at uint256.max.
0xRajeev
It is good to add a require() statement that checks the return value of arbitrary token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token returns a boolean or reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contracts. None of the contracts use the safe versions. This should at least be used with user-specified external tokens.
Rationale for severity: Reference this similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call
Manual Analysis
Add require()s or better use the safe* versions from OpenZeppelin.
#0 - SamusElderg
2021-07-22T03:09:23Z
Duplicate of #8
1773.9026 USDC - $1,773.90
0xRajeev
When a member calls removeLiquiditySingle() requesting only SPARTA in return, i.e. toBASE = true, the LP tokens are transferred to the Pool to withdraw the constituent SPARTA and TOKENs back to the Router. The withdrawn TOKENs are then transferred back to the Pool to convert to SPARTA and directly transferred to the member from the Pool. However, the member’s SPARTA are left behind in the Router instead of being returned along with converted SPARTA from the Pool.
In other words, the _member's BASE SPARTA tokens that were removed from the Pool along with the TOKENs are never sent back to the _member because the _token's transferred to the Pool are converted to SPARTA and only those are sent back to member directly from the Pool via swapTo().
This effectively results in member losing the SPARTA component of their Pool LP tokens which get left behind in the Router and are possibly claimed by future transactions that remove SPARTA from Router.
LPs sent to Pool: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L121
SPARTA and TOKENs withdrawn from Pool to Router: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L122
TOKENs from Router sent to Pool: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L126
TOKENs in Pool converted to BASE SPARTA and sent to member directly from the Pool: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Router.sol#L127
Manual Analysis
#0 - verifyfirst
2021-07-22T01:56:31Z
This bug was missed in a last minute edit before pushing to code423n4, wouldn't have made it past testNet testing. However, it is a good find.
0xRajeev
There is no input validation of any kind (zero/sanity/threshold) on any of the proposal parameters such as typeStr, param, proposedAddress, recipient and amount to see if they make sense in the context of what is supported by the DAO.
Proposal submitters lose their proposal fee at the cost of invalid proposals. Zero address/value checks are postponed to the last stage of completion (after voting and finalization) which wastes time, gas and might make voters not vote at all leading to dangling proposals waiting for cancellation period of 15 days.
Manual Analysis
Perform relevant validation of input parameters of new proposals to reject any parameters that are clearly irrelevant/incorrect e.g. unsupported typeStr, zero address, zero param, zero grant amount, listing/delisting bonds that are already in the state being proposed, adding/removing curated pools that are already in the state being proposed. These should be performed in the new*Proposal() functions instead of the proposal action functions which get executed after a successful vote. Input validation should happen closest to the interface where inputs are received instead of being postponed to after voting/finalisation.
#0 - SamusElderg
2021-07-23T06:33:27Z
Duplicate of #43
0xRajeev
Unnecessary/incorrect access control modifier is typically an indication of missing critical authorization checks. The onlyDAO modifier (used in various protocol contracts) is present in synthFactory.sol but used only in the purgeDeployer (which sets deployer to 0 address) and is also incorrect in that it only checks against DEPLOYER and not the DAO address. This is effectively unnecessary.
Manual Analysis
Re-evaluate access control logic in modifier and purgeDeployer() to either use this modifier for other functions or else remove it along with purgeDeployer().
#0 - SamusElderg
2021-07-23T08:31:43Z
Duplicate of #172
#1 - ghoul-sol
2021-08-08T19:24:52Z
Duplicate of #172
0xRajeev
The harvest() function operates on msg.sender who may not be the same as member used in depositLPForMember(). A griefing attacker (msg.sender) can target an arbitrary member (with existing weight) by depositing some dust LP which gets accounted towards member's deposits, potentially accumulates harvest rewards (in the harvest function) and then forces reset of member's last harvest time on L166 which prevents the actual member from accumulating harvest rewards for that duration when member calls deposit() or depositLPForMember() directly. The member thus loses harvest rewards/incentives for that duration because of the lastTime reset.
This is a different vector compared to the other similar finding where using a relayer leads to member missing harvest rewards, because this vector does not depend on the use of a relayer at all.
Also, this is a griefing attack where the attacker potentially does not get any benefits/rewards unless it is another participating member in the protocol.
Manual Analysis
Pass an address parameter to harvest() and use that instead of msg.sender
#0 - SamusElderg
2021-07-26T02:10:48Z
Duplicate of #235
0xRajeev
In claimForMember, the member claims back some of their bonded LPs. The check to see if claimRate can be made 0 should preceed the _claimable deduction on L110. This misplaced check after deduction leads to incorrect zero-ing of member’s non-zero claimable bonded LPs i.e. loss of funds. This happens whenever member claims half of their bonded LPs.
Scenario: Say bondedLP[member] = 400 before duduction and _claimable happens to be 200. Then, after L110, bondedLP[member] = 400 - 200 = 200, which satisfies the predicate on L111 because bondedLP[member] == _claimable == 200. This leads to claimRate[member] being set to 0 although member still has a remainder 200 bonded LPs yet to claim.
Manual Analysis
Move L110 deduction to after L113. The conditional check and setting of claimRate[member] to zero should happen before the deduction.
#0 - verifyfirst
2021-07-22T01:53:58Z
This is a duplicate with #42
#1 - ghoul-sol
2021-08-08T22:06:38Z
duplicate of #42 so medium risk
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
Attackers, i.e. malicious DAO members, can grief voting by removing their votes just before finalization and if that takes it below quorum, it forces others to vote and restarts another cooloff period of 3 days. This will delay the finalisation of targeted proposals and the griefing attackers lose nothing in penalty.
Manual Analysis
#0 - SamusElderg
2021-07-23T09:34:38Z
I don't agree with locking up voters; as we all know people like to press buttons and sometimes form an opinion later; need to allow them to change their mind (or have their mind changed by good discussion)
However, a fee/penalty of sorts for removeVote is an interesting discussion point 👍 @verifyfirst
#1 - verifyfirst
2021-07-26T02:10:34Z
I agree with the suggestion of a removeVote penalty to reduce the unlikely grieving
0xRajeev
The payable attribute will allow swapTo() to receive BNB and may confuse/allow users into sending BNB to contract trying to swap that to token, which is not the case. This will lead to user's BNB getting locked in this contract without a way to retrieve it.
Manual Analysis
Remove payable attribute as it is not required here.
#0 - SamusElderg
2021-07-26T03:41:25Z
Duplicate of #46
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
zapLiquidity() used to trade LP tokens of one pool to another is missing a check for toPool != fromPool which may happen accidentally. The check will prevent unnecessary transfers and avoid any fees/slippage or accounting errors.
Manual Analysis
Add toPool != fromPool as part of input validation.
#0 - verifyfirst
2021-07-26T01:59:39Z
Although very low risk, recommended mitigation is valid
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
Incorrect event parameter outputAmount is used (instead of output) in the MintSynth event emit. outputAmount is a named return variable that is never set in this function and so will always be 0. This should instead be output. This will confuse the UI or offchain monitoring tools that 0 synths were minted and will lead to users panicking/complaining or trying to mint synth again.
Manual Analysis
Replace outputAmount with output in the emit.
#0 - verifyfirst
2021-07-26T02:05:27Z
Code needs refactoring
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
The threshold check for basisPoints while a required part of input validation is an unnecessary redundant check because calcPart() does a similar upper bound check and the lower bound check on 0 is only an optimization.
Manual Analysis
Remove redundant check to save gas and improve readability/maintainability.
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
This isListedPool check implemented by isPool() is missing in many functions of the contract that accept pool/token addresses from users. getPool() returns the default mapping value of 0 for token that do not have valid pools. This lack of input validation may lead to use of zero/invalid pool addresses in the protocol context and reverts in the best case or burn/loss of user funds in the worst case.
Use of getPool() without isPool() check: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/BondVault.sol#L108
Several usages of getPool() in Utils.sol and other places.
Manual Analysis
Combine isPool() isListedPool check to getPool() so that it always returns a valid/listed pool in the protocol.
#0 - verifyfirst
2021-07-26T01:57:28Z
Code can be cleaner
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
Without a setter for curatedPoolSize, the number of curated pools at any point can only be a max of 10 forever, and will require removing one to accommodate another one. It is unclear if this is intentional and a requirement of the protocol.
Manual Analysis
Recommend a setter for curatedPoolSize that allows DAO to increase it if/when required. If not, document the hardcoded limit of curated pools number.
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
The token argument used in CreatePool event emit of createPoolADD() should really be _token so that WBNB address is logged in the event instead of zero address when token == 0. Logging a zero address could confuse off-chain user interfaces because it is treated as a burn address by convention.
Manual Analysis
Use _token instead of token in event emit.
#0 - SamusElderg
2021-07-26T10:06:24Z
Non-critical, but makes sense; will change this 👍
0xRajeev
There is no reason to expose the burnSynth() function externally to anyone other than curated pools (similar to mintSynth) although it is unlikely to facilitate any attack vectors.
Manual Analysis
In guidance of the least privilege security principle, add onlyPool modifier to the function.
#0 - SamusElderg
2021-07-26T01:37:06Z
Duplicate of #70
177.3903 USDC - $177.39
0xRajeev
addCuratedPool() is missing a require(isCuratedPool[_pool] == false) check, similar to the one in removeCuratedPool to ensure that the DAO is not trying to curate an already curated pool which indicates a mismatch of assumption/accounting compared to the contract state.
Manual Analysis
Add require(isCuratedPool[_pool] == false) before setting isCuratedPool[_pool] = true.
#0 - SamusElderg
2021-07-26T02:23:09Z
Check needs to be added for this issue
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
The DAO deployer is used as the authorized address in the modifier onlyDAO allowing it to set various critical protocol addresses and parameters. The purgeDeployer() function is expected to be called by the deployer once the DAO is stable and final. However, a single-step critical action such as this is extremely risky because it may be called accidentally and is irrecoverable from such mistakes.
Scenario 1: The DAO is not yet stable and final. But the deployer, e.g. controlled by an EOA, accidentally triggers this function. The protocol parameters/addresses can no longer be changed when required. The entire protocol has to be halted and redeployed. User funds have to be returned. Protocol reputation takes a hit.
Scenario 2: The DAO is not yet stable and final but the deployer incorrectly assumes it is final and triggers this function. The protocol parameters/addresses can no longer be changed when required. The entire protocol has to be halted and redeployed. User funds have to be returned. Protocol reputation takes a hit.
While a two-step process is generally recommended for critical address changes, a single-step purge/renounce is equally risky if it is controlled by an EOA and is not timelocked.
Manual Analysis
At a minimum, make sure that 1) deployer is not an EOA but a multisig controlled by mutually independent and trustworthy entities 2) this function is timelocked.
A better design change would be to let the DAO decide if it considers itself stable/final and let it vote for a proposal that purges the deployer.
#0 - verifyfirst
2021-07-23T07:52:08Z
Purge deployer is something that would be completed once the protocol is completely stable and future proof. The risk of accidentally calling purgeDeployer is very low. Suggested mitigations are considered.
#1 - ghoul-sol
2021-08-08T21:24:40Z
Possibility of this happening is very low so making this low risk
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places. Given that zero-address is used as an indicator for BNB, there is a greater risk of using it accidentally.
Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.
Manual Analysis
Add zero-address checks for all initializations/setters of all address state variables.
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
An attacker can front-run any operation that depends on the pool contract's internal balance amounts being unsynced to pool's balance on token/base contracts effectively nullifying the transfer of base/tokens for those operations. This will make _getAddedBaseAmount() and _getAddedTokenAmount() return 0 (because the balances are synced) from such operations.
Impact: The affected operations are: addForMember(), swapTo() and mintSynth() which will all take the user funds to respective contracts but will treat it as 0 (because of the syncing) and thus not add liquidity, return swapped tokens or mint any synths to the affected users. User loses deposited funds to the contract.
Manual Analysis
Add access control to sync() function so that only Router can call it via addDividend().
#0 - verifyfirst
2021-07-22T01:51:16Z
Whilst we disagree with the above attack vector, it brings up a point about permissions on the pool's sync() function which was always intended to be called by anyone incase of accidentally send in. However, we have decided to permission the sync to router only just for peace of mind.
#1 - SamusElderg
2021-07-30T05:20:57Z
To be clear; this is non-critical based on the warden's outlined scenario. Front-running a user's txn would mean sync() is called before the user's funds are sent in, so sync() would have no effect on a txn that hasn't happened yet. Unpermissioned sync() Might however have low risk or otherwise in other scenarios but cant simulate or think of any. Regardless we will permission sync() to close any vector that has not been thought of there.
#2 - ghoul-sol
2021-08-08T22:05:33Z
Per sponsor comment, I align with low risk
🌟 Selected for report: 0xRajeev
160 USDC - $160.00
0xRajeev
The state variables corresponding to setGenesisFactors() parameters _coolOff, _daysToEarn, _majorityFactor, _daoClaim and_daoFee are declared to be uint256 but are set using these parameters that are uint32. While it’s unlikely that these will need values > uint32, this leads to wastage of storage slots and gas.
Manual Analysis
The state variables can be declared uint32 to fit all five of them in a single slot and this will lead to efficient SSTOREs because they are set together. If values > uint32 are relevant, then the parameter types of setter setGenesisFactors() have to be changed.
#0 - SamusElderg
2021-07-26T01:49:44Z
Can definitely optimize here; but this isn't a risk-category but rather gas optimization
#1 - ghoul-sol
2021-08-08T19:46:40Z
gas optimization indeed.
🌟 Selected for report: 0xRajeev
394.2006 USDC - $394.20
0xRajeev
Event log poisoning is possible by griefing attackers who have no DAO weight but vote and emit event that takes up event log space.
Manual Analysis
Emit event only if non-zero weight as relevant to proposal voting/cancelling.
#0 - SamusElderg
2021-07-23T09:31:47Z
Good point; @verifyfirst should we make the event conditional or is it used anywhere when the vote is zeroed out? From memory when zeroed-out it is simply done via mappings and doesn't emit an event anyway so probably safe to conditional this one (or remove it if we aren't using it in any user-facing interface)
#1 - verifyfirst
2021-07-26T02:12:19Z
Yep, a conditional is a good one