Spartan Protocol contest - shw's results

Handle

shw

Vulnerability details

Impact

The _approve functions of the pool LP tokens and synths do nothing if the _allowances is already the maximum number, i.e., type(uint256).max. Therefore, Alice cannot change her allowance to Bob once she approved him with the maximum approval.

Proof of Concept

Referenced code: Pool.sol#L99 Synth.sol#L93

Recommended Mitigation Steps

Consider removing the _allowances[owner][spender] < type(uint256).max condition of _approve to allow users to reset their allowance to others even if it is the maximum.

Handle

shw

Vulnerability details

Impact

The return values of BEP20.transfer and BEP20.transferFrom are not checked to be true in multiple contracts. The return value could be false if the transferred token is not BEP20-compliant, indicating that the transfer fails, while the calling contract will not notice the failure. As written in the BEP20 specification:

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Proof of Concept

The return value check should be applied, especially when the transferred token is not created by the Spartan protocol (and thus could be non-compliant to BEP20):

Unchecked transfer: Pool.sol#L199 Pool.sol#L224 Router.sol#L67 Router.sol#L126 Router.sol#L221

Unchecked transferFrom: poolFactory.sol#L112 Router.sol#L207 Synth.sol#L204 Dao.sol#L266

Recommended Mitigation Steps

Use the SafeBEP20 implementation from PancakeSwap (or SafeERC20 from OpenZeppelin) and call safeTransfer or safeTransferFrom when transferring BEP20 tokens.

Handle

shw

Vulnerability details

Impact

The getPoolShareWeight function returns a user's pool share weight by calculating how many SPARTAN the user's LP tokens account for. However, this approach is vulnerable to flash loan manipulation since an attacker can swap a large number of TOKEN to SPARTAN to increase the number of SPARTAN in the pool, thus effectively increasing his pool share weight.

Proof of Concept

According to the implementation of getPoolShareWeight, a user's pool share weight is calculated by uints * baseAmount / totalSupply, where uints is the number of user's LP tokens, totalSupply is the total supply of LP tokens, and baseAmount is the number of SPARTAN in the pool. Thus, a user's pool share weight is proportional to the number of SPARTAN in the pool. Consider the following attack scenario:

1. Supposing the attacked pool is SPARTAN-WBNB. The attacker first prepares some LP tokens (WBNB-SPP) by adding liquidity to the pool.
2. The attacker then swaps a large number of WBNB to SPARTAN, which increases the pool's baseAmount. He could split his trade into small amounts to reduce slip-based fees.
3. The attacker now wants to increase his weight in the DaoVault. He adds his LP tokens to the pool by calling the deposit function of Dao.
4. Dao then calls depositLP of DaoVault, causing the attacker's weight to be recalculated. Due to the large proportion of SPARTAN in the pool, the attacker's weight is artificially increased.
5. With a higher member weight, the attacker can, for example, vote the current proposal with more votes than he should have or obtain more rewards when calling harvest of the Dao contract.
6. The attacker then swaps back SPARTAN to WBNB and only loses the slip-based fees.

Referenced code: Utils.sol#L46-L50 Utils.sol#L70-L77 DaoVault.sol#L44-L56 Dao.sol#L201 Dao.sol#L570

Recommended Mitigation Steps

A possible mitigation is to record the current timestamp when a user's weight in the DaoVault or BondVault is recalculated and force the new weight to take effect only after a certain period, e.g., a block time. This would prevent the attacker from launching the attack since there is typically no guarantee that he could arbitrage the WBNB back in the next block.

Handle

shw

Vulnerability details

Impact

The functions of creating new DAO proposals (e.g., newActionProposal) are permissionless. Anyone can create a new proposal by paying some fees in SPARTA, as long as the previous proposal is closed. Thus, an attacker could then front-run proposals of benign users to prevent their proposals from being created. Moreover, the only way to close the malicious proposal without completing it is to wait for it to expire (15 days long). The attacker only needs to front-run benign DAO proposals every 15 days to effectively perform a DoS attack on the DAO.

Proof of Concept

Referenced code: Dao.sol#L310 Dao.sol#L319 Dao.sol#L329 Dao.sol#L339 Dao.sol#L352 Dao.sol#L407

Recommended Mitigation Steps

Add a function that allows the deployer to remove malicious proposals directly in case this attack happens.

Handle

shw

Vulnerability details

Impact

Some critical parameters (e.g., the maximum number of pools that can be curated) of the Spartan protocol are hard-coded in the contracts and thus unchangeable after the contracts are deployed. Allowing admins to adjust such parameters dynamically makes the code easier to maintain and adds flexibility to the protocol without re-deploying the related contracts.

Proof of Concept

Referenced code: Dao.sol#L407 DaoVault.sol#L69 poolFactory.sol#L31

Recommended Mitigation Steps

Consider adding a function with the onlyDAO modifier to allow the deployer or DAO to change these parameters in case of need.

Handle

shw

Vulnerability details

Impact

The claimAllForMember function of Dao is permissionless, allowing anyone to claim the unlocked bonded LP tokens for any member. However, claiming a member's LP tokens could decrease the member's weight in the BondVault, thus affecting the member's votes and rewards in the Dao contract.

Proof of Concept

For example, an attacker can intentionally front-run a victim's voteProposal call to decrease the victim's vote weight to prevent the proposal from being finalized:

1. Supposing the victim's member weight in the BondVault is 201, the total weight is 300. The victim has some LP tokens claimable from the vault, and if claimed, the victim's weight will be decreased to 101. To simplify the situation, assuming that the victim's weight in the DaoVault and the total weight of the DaoVault are both 0.
2. The victim wants to vote on the current proposal, which requires the majority consensus. If the victim calls voteProposal, the proposal will be finalized since the victim has the majority weight (201/300 > 66.6%).
3. An attacker does not want the proposal to be finalized, so he calls claimAllForMember with the victim as the parameter to intentionally decrease the victim's weight.
4. As a result, the victim's weight is decreased to 101, and the total weight is decreased to 200. The victim cannot finalize the proposal since he has no majority anymore (101/200 < 66.6%).

Similarly, an attacker can front-run a victim's harvest call to intentionally decrease the victim's reward since the amount of reward is calculated based on the victim's current weight.

Referenced code: Dao.sol#L179-L206 Dao.sol#L276-L285 Dao.sol#L369-L383 Dao.sol#L568-L574 Dao.sol#L577-L586 BondVault.sol#L104-L117 BondVault.sol#L120-L129 BondVault.sol#L155-L162

Recommended Mitigation Steps

Consider removing the member parameter in the claimAllForMember function and replace all member to msg.sender to allow only the user himself to claim unlocked bonded LP tokens.