Spartan Protocol contest - cmichel's results

Handle

cmichel

Vulnerability details

Vulnerability Details

The SynthVault.withdraw function does not claim the user's rewards. It decreases the user's weight and therefore they are forfeiting their accumulated rewards. The synthReward variable in _processWithdraw is also never used - it was probably intended that this variable captures the claimed rewards.

Impact

Usually, withdrawal functions claim rewards first but this one does not. A user that withdraws loses all their accumulated rewards.

Recommended Mitigation Steps

Claim the rewards with the user's deposited balance first in withdraw.

Handle

cmichel

Vulnerability details

The Pool._approve function performs a no-op if the allowance is currently set to type(uint256).max. This leads to the issue that approvals cannot be changed anymore once they are set to the max value.

Impact

Imagine someone approving an operator with the max value but their key is compromised. The approval cannot be revoked anymore. This is especially bad as any approveAndCall call sets the max approval.

Recommended Mitigation Steps

The correct check to save gas should be if (_allowances[owner][spender] != amount) { update }.

Handle

cmichel

Vulnerability details

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. This is generally not an issue when the verified, deployed, reverting Pool token is used, but it can be for other tokens.

transferFrom occurences where arbitrary tokens are used:

• Router._handleTransferIn (iBEP20(_token).transferFrom)
• DAO._handleTransferIn
• poolFactory._handleTransferIn
• Synth._handleTransferIn

transfer occurences where arbitrary tokens are used:

• Router._handleTransferOut (iBEP20(_token).transfer)
• Router.zapLiquidity
• Router.removeLiquiditySingle
• Pool.removeForMember
• Pool.swapTo

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

The Pool.removeLiquiditySingle function redeems liquidity tokens for underlying to the router contract. If toBase == true, it then tries to convert these to base tokens. However, only the swapped token -> BASE amount is sent to the user, the redeemed part is still in the router and can be stolen by any other user.

Recommended Mitigation Steps

Send the entire BASE amount of the router to the member instead of only the swapped amount.

Handle

cmichel

Vulnerability details

Vulnerability Details

The SynthVault._deposit function adds weight for the user that depends on the spot value of the deposit synth amount in BASE. This spot price can be manipulated and the cost of manipulation is relative to the pool's liquidity. However, the reward (see calcReward) is measured in BASE tokens unrelated to the pool. Therefore, if the pool's liquidity is low and the reward reserve is high, the attack can be profitable:

1. Manipulate the pool spot price of the iSYNTH(_synth).LayerONE() pool by dripping a lot of BASE into it repeatedly (sending lots of smaller trades is less costly due to the path-independence of the continuous liquidity model). This increases the BASE per token price.
2. Call SynthVault.depositForMember and deposit a small amount of synth token. The iUTILS(_DAO().UTILS()).calcSpotValueInBase(iSYNTH(_synth).LayerONE(), _amount) will return an inflated weight due to the price.
3. Optionally drip more BASE into the pool and repeat the deposits
4. Drip back token to the pool to rebalance it

The user's weight is now inflated compared to the deposited / locked-up amount and they can claim a large share of the rewards.

Impact

The cost of the attack depends on the pool's liquidity and the profit depends on the reserve. It could therefore be profitable under certain circumstances.

Recommended Mitigation Steps

Track a TWAP price of the synth instead, store the deposited synths instead, and compute the weight & total weight on the fly based on the TWAP * deposit amount instead of at the time of deposit.

Handle

cmichel

Vulnerability details

The Router (and Pool) does not implement any slippage checks with comparing the swap / liquidity results with a minimum swap / liquidity value.

Impact

Users can be frontrun and receive a worse price than expected when they initially submitted the transaction. There's no protection at all, no minimum return amount or deadline for the trade transaction to be valid which means the trade can be delayed by miners or users congesting the network, as well as being sandwich attacked - ultimately leading to loss of user funds.

Recommended Mitigation Steps

Add some sort of protection for the user such that they receive their desired amounts. Add a minimum return amount for all swap and liquidity provisions/removals to all Router functions.

Handle

cmichel

Vulnerability details

The Router.addDividend function tells the reserve to send dividends to the pool depending on the fees.

• The attacker provides LP to a curated pool. Ideally, they become a large LP holder to capture most of the profit, they should choose the smallest liquidity pool as the dividends are pool-independent.
• The normalAverageFee variable that determines the pool dividends can be set to zero by the attacker by trading a single wei in the pool arrayFeeSize (20) times (use buyTo). The fees of the single wei trades will be zero and thus the normalAverageFee will also be zero as, see addTradeFee.
• The attacker then does a trade that generates some non-zero fees, setting the normalAverageFee to this trade's fee. The feeDividend is then computed as _fees * dailyAllocation / (_fees + normalAverageFee) = _fees * dailyAllocation / (2 * _fees) = dailyAllocation / 2. Half of the dailyAllocation is sent to the pool.
• The attacker repeats the above steps until the reserve is almost empty. Each time the dailyAllocation gets smaller but it's still possible to withdraw almost all of it.
• They redeem their LP tokens and gain a share of the profits

Impact

The reserve can be emptied by the attacker.

Recommended Mitigation Steps

Counting only the last 20 trades as a baseline for the dividends does not work. It should probably average over a timespan but even that can be gamed if it is too short. I think a better idea is to compute the dividends based on volume traded over a timespan instead of looking at individual trades.

Handle

cmichel

Vulnerability details

Several contracts implement an onlyDAO modifier which, as the name suggests, should only authorize the function to be executed by the DAO. However, some implementations are wrong and either allow the DAO or the deployer to execute, or even only the deployer:

Incorrect implementations:

• BondVault.onlyDAO: allows deployer + DAO
• DAO.onlyDAO: allows deployer
• DAOVault.onlyDAO: allows deployer + DAO
• poolFactory.onlyDAO: allows deployer + DAO
• Router.onlyDAO: allows deployer + DAO
• Synth.onlyDAO: allows deployer
• synthFactory.onlyDAO: allows deployer
• synthVault.onlyDAO: allows deployer + DAO

Impact

In all of these functions, the deployer may execute the function as well which is a centralization risk. The deployer can only sometimes be purged, as in synthFactory, in which case nobody can execute these functions anymore.

Recommended Mitigation Steps

Rename it to onlyDeployer or onlyDeployerOrDAO depending on who has access.

Handle

cmichel

Vulnerability details

Vulnerability Details

The protocol differentiates between public pool creations and private ones (starting without liquidity). However, this is not effective as anyone can just flashloan the required initial pool liquidity, call PoolFactory.createPoolADD, receive the LP tokens in addForMember and withdraw liquidity again.

Recommended Mitigation Steps

Consider burning some initial LP tokens or taking a pool creation fee instead.

Handle

cmichel

Vulnerability details

Vulnerability Details

The Pool.approveAndCall function approves the recipient contract with the max value instead of only the required amount.

Impact

For safety, the approval should not be set to the max value, especially if the amount that the contract may use is already known in this call, like this is the case for approveAndCall.

Recommended Mitigation Steps

Only approve amount.

Handle

cmichel

Vulnerability details

Vulnerability Details

The Synth.approveAndCall function approves the recipient contract with the max value instead of only the required amount.

Impact

For safety, the approval should not be set to the max value, especially if the amount that the contract may use is already known in this call, like this is the case for approveAndCall.

Recommended Mitigation Steps

Only approve amount.

Handle

cmichel

Vulnerability details

Vulnerability Details

The SynthVault.harvestSingle function can be used to mint & deposit synths without using a lockup. An attacker sends BASE tokens to the pool and then calls harvestSingle. The inner iPOOL(_poolOUT).mintSynth(synth, address(this)); call will mint synth tokens to the vault based on the total BASE balance sent to the pool, including the attacker's previous transfer. They are then credited the entire amount to their weight.

This essentially acts as a (mint +) deposit without a lock-up period.

Recommended Mitigation Steps

Sync the pool before sending BASE to it through iRESERVE(_DAO().RESERVE()).grantFunds(reward, _poolOUT); such that any previous BASE transfer is wasted. This way only the actual reward's weight is increased.

Handle

cmichel

Vulnerability details

The PoolFactory.curatedPoolCount iterates over all arrayPools. Anyone can push to this array by creating a pool making this attack easy to execute for an attacker.

Impact

The transactions can fail if the arrays get too big and the transaction would consume more gas than the block limit. No pool can then be curated anymore.

Recommended Mitigation Steps

Count the pool in a separate variable poolCount increasing/decreasing in add/removeCuratedPool. This also saves lots of gas for users creating pools.

Handle

cmichel

Vulnerability details

BondVault deposits match any deposited token amount with the BASE amount to provide liquidity, see Docs and DAO.handleTransferIn. The matched BASE amount is the swap amount of the token trade in the pool. An attacker can manipulate the pool and have the DAO commit BASE at bad prices which they then later buys back to receive a profit on BASE. This is essentially a sandwich attack abusing the fact that one can trigger the DAO to provide BASE liquidity at bad prices:

1. Manipulate the pool spot price by dripping a lot of BASE into it repeatedly (sending lots of smaller trades is less costly due to the path-independence of the continuous liquidity model). This increases the token per BASE price.
2. Repeatedly call DAO.bond(amount) to drip tokens into the DAO and get matched with BASE tokens to provide liquidity. (Again, sending lots of smaller trades is less costly.) As the pool contains low token but high BASE reserves, the spartaAllocation = _UTILS.calcSwapValueInBase(_token, _amount) swap value will be high. The contract sends even more BASE to the pool to provide this liquidity.
3. Unmanipulate the pool by sending back the tokens from 1. As a lot more BASE tokens are in the reserve now due to the DAO sending it, the attacker will receive more BASE as in 1. as well, making a profit

Impact

The DAO's Bond allocation can be stolen. The cost of the attack is the trade fees in 1. + 3. as well as the tokens used in 2. to match the BASE, but the profit is a share on the BASE supplied to the pool by the DAO in 2.

Recommended Mitigation Steps

Track a TWAP spot price of the TOKEN <> BASE pair and check if the BASE incentive is within a range of the TWAP. This circumvents that the DAO commits BASE at bad prices.