Spartan Protocol contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

A call to transferFrom or transfer is frequently done without checking the results. For certain ERC20 tokens, if insufficient tokens are present, no revert occurs but a result of "false" is returned. So its important to check this. If you don't you could mint tokens without have received sufficient tokens to do so. So you could loose funds.

Its also a best practice to check this. See below for example where the result isn't checked.

Note, in some occasions the result is checked (see below for examples).

Proof of Concept

Highest risk: .\Dao.sol: iBEP20(_token).transferFrom(msg.sender, address(this), _amount); // Transfer user's assets to Dao contract .\Pool.sol: iBEP20(TOKEN).transfer(member, outputToken); // Transfer the TOKENs to user .\Pool.sol: iBEP20(token).transfer(member, outputAmount); // Transfer the swap output to the selected user .\poolFactory.sol: iBEP20(_token).transferFrom(msg.sender, _pool, _amount); .\Router.sol: iBEP20(_fromToken).transfer(fromPool, iBEP20(_fromToken).balanceOf(address(this))); // Transfer TOKENs from ROUTER to fromPool .\Router.sol: iBEP20(_token).transfer(_pool, iBEP20(_token).balanceOf(address(this))); // Transfer TOKEN to pool .\Router.sol: iBEP20(_token).transferFrom(msg.sender, _pool, _amount); // Transfer TOKEN to pool .\Router.sol: iBEP20(_token).transfer(_recipient, _amount); // Transfer TOKEN to recipient .\Synth.sol: iBEP20(_token).transferFrom(msg.sender, address(this), _amount); // Transfer tokens in

less risky .\Router.sol: iBEP20(fromPool).transferFrom(_member, fromPool, unitsInput); // Transfer LPs from user to the pool .\BondVault.sol: iBEP20(_pool).transfer(member, _claimable); // Send claim amount to user .\Router.sol: iBEP20(_pool).transferFrom(_member, _pool, units); // Transfer LPs to the pool .\Router.sol: iBEP20(_pool).transferFrom(_member, _pool, units); // Transfer LPs to pool .\Router.sol: iBEP20(fromSynth).transferFrom(msg.sender, _poolIN, inputAmount); // Transfer synth from user to pool .\Pool.sol: iBEP20(synthIN).transfer(synthIN, _actualInputSynth); // Transfer SYNTH to relevant synth contract .\Router.sol: iBEP20(WBNB).transfer(_pool, _amount); // Transfer WBNB from ROUTER to pool .\Dao.sol: iBEP20(BASE).transfer(newDAO, baseBal); .\Pool.sol: iBEP20(BASE).transfer(member, outputBase); // Transfer the SPARTA to user .\Pool.sol: iBEP20(BASE).transfer(member, outputBase); // Transfer SPARTA to user .\Router.sol: iBEP20(BASE).transfer(toPool, iBEP20(BASE).balanceOf(address(this))); // Transfer SPARTA from ROUTER to toPool .\Router.sol: iBEP20(BASE).transfer(_pool, iBEP20(BASE).balanceOf(address(this))); // Transfer SPARTA to pool .\Router.sol: iBEP20(BASE).transfer(_pool, iBEP20(BASE).balanceOf(address(this))); // Transfer SPARTA from ROUTER to pool .\Router.sol: iBEP20(BASE).transferFrom(msg.sender, _pool, inputAmount); // Transfer SPARTA from ROUTER to pool

Sometimes the result is checked: .\Dao.sol: require(iBEP20(pool).transferFrom(msg.sender, address(_DAOVAULT), amount), "!funds"); // Send user's deposit to the DAOVault .\Dao.sol: require(iBEP20(BASE).transferFrom(msg.sender, address(_RESERVE), _amount), '!fee'); // User pays the new proposal fee .\DaoVault.sol: require(iBEP20(pool).transfer(member, _balance), "!transfer"); // Transfer user's balance to their wallet .\synthVault.sol: require(iBEP20(synth).transferFrom(msg.sender, address(this), amount)); // Must successfuly transfer in .\synthVault.sol: require(iBEP20(synth).transfer(msg.sender, redeemedAmount)); // Transfer from SynthVault to user

Tools Used

grep

Recommended Mitigation Steps

Always check the result of transferFrom and transfer

Handle

gpersoon

Vulnerability details

Impact

You can keep the DAO voting system hostage by repeatedly adding useless proposals. You would have to monitor the blockchain and as soon as a new proposal can be made, make a new useless proposal. (submit with a high fee and/or via a high priority channel like flashbots, taichi, bloxroute, archer)

A new proposal can only be submitted when mapPID_open[currentProposal] == false (see checkProposal() ) The contract can only reach this state via cancelProposal() or finaliseProposal()&completeProposal() Both cancelProposal() and finaliseProposal() force waiting for quite a while:

• cancelProposal ==> 1296000 seconds (15 days)
• finaliseProposal ==> coolOffPeriod ==> 259200 seconds (3 days)

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Dao.sol#L406

function checkProposal() internal { require(mapPID_open[currentProposal] == false, '!open'); // There must not be an existing open proposal proposalCount += 1; // Increase proposal count currentProposal = proposalCount; // Set current proposal to the new count mapPID_open[currentProposal] = true; // Set new proposal as open status mapPID_startTime[currentProposal] = block.timestamp; // Set the start time of the proposal to now }

function cancelProposal() external { require(block.timestamp > (mapPID_startTime[currentProposal] + 1296000), "!days"); // Proposal must not be new mapPID_votes[currentProposal] = 0; // Clear all votes from the proposal mapPID_open[currentProposal] = false; // Set the proposal as not open (closed status) emit CancelProposal(msg.sender, currentProposal); }

// A finalising-stage proposal can be finalised after the cool off period
function finaliseProposal() external {
    require((block.timestamp - mapPID_coolOffTime[currentProposal]) > coolOffPeriod, "!cooloff"); // Must be past cooloff period
    require(mapPID_finalising[currentProposal] == true, "!finalising"); // Must be in finalising stage
    if(!hasQuorum(currentProposal)){
        mapPID_finalising[currentProposal] = false; // If proposal has lost quorum consensus; kick it out of the finalising stage
    } else {
        bytes memory _type = bytes(mapPID_type[currentProposal]); // Get the proposal type
        if(isEqual(_type, 'DAO')){
            moveDao(currentProposal);
        } else if (isEqual(_type, 'ROUTER')) {
            moveRouter(currentProposal);
        } else if (isEqual(_type, 'UTILS')){
            moveUtils(currentProposal);
        } else if (isEqual(_type, 'RESERVE')){
            moveReserve(currentProposal);
        } else if (isEqual(_type, 'FLIP_EMISSIONS')){
            flipEmissions(currentProposal);
        } else if (isEqual(_type, 'COOL_OFF')){
            changeCooloff(currentProposal);
        } else if (isEqual(_type, 'ERAS_TO_EARN')){
            changeEras(currentProposal);
        } else if (isEqual(_type, 'GRANT')){
            grantFunds(currentProposal);
        } else if (isEqual(_type, 'GET_SPARTA')){
            _increaseSpartaAllocation(currentProposal);
        } else if (isEqual(_type, 'LIST_BOND')){
            _listBondingAsset(currentProposal);
        } else if (isEqual(_type, 'DELIST_BOND')){
            _delistBondingAsset(currentProposal);
        } else if (isEqual(_type, 'ADD_CURATED_POOL')){
            _addCuratedPool(currentProposal);
        } else if (isEqual(_type, 'REMOVE_CURATED_POOL')){
            _removeCuratedPool(currentProposal);
        } 
    }
}

function completeProposal(uint _proposalID) internal { string memory _typeStr = mapPID_type[_proposalID]; // Get proposal type emit FinalisedProposal(msg.sender, _proposalID, mapPID_votes[_proposalID], _DAOVAULT.totalWeight(), _typeStr); mapPID_votes[_proposalID] = 0; // Reset proposal votes to 0 mapPID_finalised[_proposalID] = true; // Finalise the proposal mapPID_finalising[_proposalID] = false; // Remove proposal from 'finalising' stage mapPID_open[_proposalID] = false; // Close the proposal }

Tools Used

Recommended Mitigation Steps

Allow multiple simultaneous proposals Make it easier to cancel proposals Require a stake to enter proposals (which might be slashed)

Handle

gpersoon

Vulnerability details

Impact

The contract DAO.sol has a function to remove the DEPLOYER, e.g. purgeDeployer() This means that the function with the modifier onlyDAO can no longer be executed Note: the name "onlyDAO" is misleading because it doesn't mean the DAO but the deployer of the DAO

Some functions might still be useful, also when upgrading the DAO contract. These could be:

• changeBondingPeriod
• burnBalance
• moveBASEBalance However these functions cannot be executed via the DAO itself (e.g. using the finaliseProposal() )

Proof of Concept

// https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Dao.sol#L137 function purgeDeployer() external onlyDAO { DEPLOYER = address(0); }

modifier onlyDAO() { require(msg.sender == DEPLOYER); _; }

function changeBondingPeriod(uint256 bondingSeconds) external onlyDAO{ bondingPeriodSeconds = bondingSeconds; }

function burnBalance() external onlyDAO returns (bool){ uint256 baseBal = iBEP20(BASE).balanceOf(address(this)); iBASE(BASE).burn(baseBal);
return true; }

// Can transfer the SPARTA remaining in this contract to a new DAO (If DAO is upgraded)
function moveBASEBalance(address newDAO) external onlyDAO {
    uint256 baseBal = iBEP20(BASE).balanceOf(address(this));
    iBEP20(BASE).transfer(newDAO, baseBal);
}

function finaliseProposal() external { .. if(isEqual(_type, 'DAO')){ moveDao(currentProposal); } else if (isEqual(_type, 'ROUTER')) { moveRouter(currentProposal); } else if (isEqual(_type, 'UTILS')){ moveUtils(currentProposal); } else if (isEqual(_type, 'RESERVE')){ moveReserve(currentProposal); } else if (isEqual(_type, 'FLIP_EMISSIONS')){ flipEmissions(currentProposal); } else if (isEqual(_type, 'COOL_OFF')){ changeCooloff(currentProposal); } else if (isEqual(_type, 'ERAS_TO_EARN')){ changeEras(currentProposal); } else if (isEqual(_type, 'GRANT')){ grantFunds(currentProposal); } else if (isEqual(_type, 'GET_SPARTA')){ _increaseSpartaAllocation(currentProposal); } else if (isEqual(_type, 'LIST_BOND')){ _listBondingAsset(currentProposal); } else if (isEqual(_type, 'DELIST_BOND')){ _delistBondingAsset(currentProposal); } else if (isEqual(_type, 'ADD_CURATED_POOL')){ _addCuratedPool(currentProposal); } else if (isEqual(_type, 'REMOVE_CURATED_POOL')){ _removeCuratedPool(currentProposal);

Tools Used

Recommended Mitigation Steps

Double check if there are additional functions that should be able to be executed by the DAO. If so, add them to finaliseProposal()

Rename the modifier onlyDAO to onlyDEPLOYER

Handle

gpersoon

Vulnerability details

Impact

When the DAO is just deployed it is relatively easy to gain a large (majority) share, by depositing a lot in the DAOVault and/of BONDVault. Then you could submit a proposal and vote it in. Luckily there is a coolOffPeriod of 3 days. But if others are not paying attention in these 3 days you might get your vote passed by voting for it with your majority share. The riskiest proposal would be to replace the DAO (moveDao), because that way you could take over everything.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Dao.sol function finaliseProposal() external { require((block.timestamp - mapPID_coolOffTime[currentProposal]) > coolOffPeriod, "!cooloff"); // Must be past cooloff period require(mapPID_finalising[currentProposal] == true, "!finalising"); // Must be in finalising stage if(!hasQuorum(currentProposal)){ mapPID_finalising[currentProposal] = false; // If proposal has lost quorum consensus; kick it out of the finalising stage } else { bytes memory _type = bytes(mapPID_type[currentProposal]); // Get the proposal type if(isEqual(_type, 'DAO')){ moveDao(currentProposal); ...

function voteProposal() external returns (uint voteWeight) { require(mapPID_open[currentProposal] == true, "!open"); // Proposal must be open status bytes memory _type = bytes(mapPID_type[currentProposal]); // Get the proposal type voteWeight = countVotes(); // Vote for proposal and recount if(hasQuorum(currentProposal) && mapPID_finalising[currentProposal] == false){ if(isEqual(_type, 'DAO') || isEqual(_type, 'UTILS') || isEqual(_type, 'RESERVE') ||isEqual(_type, 'GET_SPARTA') || isEqual(_type, 'ROUTER') || isEqual(_type, 'LIST_BOND')|| isEqual(_type, 'GRANT')|| isEqual(_type, 'ADD_CURATED_POOL')){ if(hasMajority(currentProposal)){ _finalise(); // Critical proposals require 'majority' consensus to enter finalization phase } } else { _finalise(); // Other proposals require 'quorum' consensus to enter finalization phase } } emit NewVote(msg.sender, currentProposal, voteWeight, mapPID_votes[currentProposal], string(_type)); }

function hasMajority(uint _proposalID) public view returns(bool){ uint votes = mapPID_votes[_proposalID]; // Get the proposal's total voting weight uint _totalWeight = _DAOVAULT.totalWeight() + _BONDVAULT.totalWeight(); // Get combined total vault weights uint consensus = _totalWeight * majorityFactor / 10000; // Majority > 66.6% if(votes > consensus){ return true; } else { return false; } }

// Check if a proposal has Quorum consensus
function hasQuorum(uint _proposalID) public view returns(bool){
    uint votes = mapPID_votes[_proposalID]; // Get the proposal's total voting weight
    uint _totalWeight = _DAOVAULT.totalWeight()  + _BONDVAULT.totalWeight(); // Get combined total vault weights
    uint consensus = _totalWeight / 2; // Quorum > 50%
    if(votes > consensus){
        return true;
    } else {
        return false;
    }
}

Tools Used

Recommended Mitigation Steps

Pay attention to the proposals when the DAO is just deployed. Make sure you initially have a majority vote.

Handle

gpersoon

Vulnerability details

Impact

When the DAO is upgraded via moveDao, it also updates the DAO address in BASE. However it doesn't update the DAO address in the Reserve.sol contract. This could be done with the function setIncentiveAddresses(..)

Now the next time grantFunds of DAO.sol is called, its tries to call: _RESERVE.grantFunds(...)

The grantFunds of Reserve.sol has the modifier onlyGrantor(), which checks the msg.sender == DAO. However in the mean time the DAO has been updated and Reserve.sol doesn't know about it and thus the modifier will not allow access to the function. Thus grantFunds will revert.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Dao.sol#L452 function moveDao(uint _proposalID) internal { address _proposedAddress = mapPID_address[_proposalID]; // Get the proposed new address require(_proposedAddress != address(0), "!address"); // Proposed address must be valid DAO = _proposedAddress; // Change the DAO to point to the new DAO address iBASE(BASE).changeDAO(_proposedAddress); // Change the BASE contract to point to the new DAO address daoHasMoved = true; // Set status of this old DAO completeProposal(_proposalID); // Finalise the proposal }

function grantFunds(uint _proposalID) internal {
    uint256 _proposedAmount = mapPID_param[_proposalID]; // Get the proposed SPARTA grant amount
    address _proposedAddress = mapPID_address[_proposalID]; // Get the proposed SPARTA grant recipient
    require(_proposedAmount != 0, "!param"); // Proposed grant amount must be valid
    require(_proposedAddress != address(0), "!address"); // Proposed recipient must be valid
    _RESERVE.grantFunds(_proposedAmount, _proposedAddress); // Grant the funds to the recipient
    completeProposal(_proposalID); // Finalise the proposal
}

// https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/outside-scope/Reserve.sol#L17 modifier onlyGrantor() { require(msg.sender == DAO || msg.sender == ROUTER || msg.sender == DEPLOYER || msg.sender == LEND || msg.sender == SYNTHVAULT, "!DAO"); _; }

function grantFunds(uint amount, address to) external onlyGrantor { .... }

function setIncentiveAddresses(address _router, address _lend, address _synthVault, address _Dao) external onlyGrantor { ROUTER = _router; LEND = _lend; SYNTHVAULT = _synthVault; DAO = _Dao; }

Tools Used

Recommended Mitigation Steps

Call setIncentiveAddresses(..) when a DAO upgrade is done.

Handle

gpersoon

Vulnerability details

Impact

The function curatedPoolCount() contains a for loop over the array arrayPools. If arrayPools would be too big then the loop would run out of gas and curatedPoolCount() would revert. This would mean that addCuratedPool() cannot be executed anymore (because it calls curatedPoolCount() )

The array arrayPools can be increased in size arbitrarily by repeatedly doing the following:

• create a pool with createPoolADD() (which requires 10,000 SPARTA)
• empty the pool with remove() of Pool.sol, which gives back the SPARTA tokens These actions will use gas to perform.

Proof of Concept

//https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/poolFactory.sol#L45 function createPoolADD(uint256 inputBase, uint256 inputToken, address token) external payable returns(address pool){ require(getPool(token) == address(0)); // Must be a valid token require((inputToken > 0 && inputBase >= (10000*10**18)), "!min"); // User must add at least 10,000 SPARTA liquidity & ratio must be finite Pool newPool; address _token = token; if(token == address(0)){_token = WBNB;} // Handle BNB -> WBNB require(_token != BASE && iBEP20(_token).decimals() == 18); // Token must not be SPARTA & it's decimals must be 18 newPool = new Pool(BASE, _token); // Deploy new pool pool = address(newPool); // Get address of new pool mapToken_Pool[_token] = pool; // Record the new pool address in PoolFactory _handleTransferIn(BASE, inputBase, pool); // Transfer SPARTA liquidity to new pool _handleTransferIn(token, inputToken, pool); // Transfer TOKEN liquidity to new pool arrayPools.push(pool); // Add pool address to the pool array ..

function curatedPoolCount() internal view returns (uint){ uint cPoolCount; for(uint i = 0; i< arrayPools.length; i++){ if(isCuratedPool[arrayPools[i]] == true){ cPoolCount += 1; } } return cPoolCount; }

function addCuratedPool(address token) external onlyDAO { ... require(curatedPoolCount() < curatedPoolSize, "maxCurated"); // Must be room in the Curated list

//https://github.com/code-423n4/2021-07-spartan/blob/main/contracts/Pool.sol#L187 function remove() external returns (uint outputBase, uint outputToken) { return removeForMember(msg.sender); }

// Contract removes liquidity for the user
function removeForMember(address member) public returns (uint outputBase, uint outputToken) {
    uint256 _actualInputUnits = balanceOf(address(this)); // Get the received LP units amount
    outputBase = iUTILS(_DAO().UTILS()).calcLiquidityHoldings(_actualInputUnits, BASE, address(this)); // Get the SPARTA value of LP units
    outputToken = iUTILS(_DAO().UTILS()).calcLiquidityHoldings(_actualInputUnits, TOKEN, address(this)); // Get the TOKEN value of LP units
    _decrementPoolBalances(outputBase, outputToken); // Update recorded BASE and TOKEN amounts
    _burn(address(this), _actualInputUnits); // Burn the LP tokens
    iBEP20(BASE).transfer(member, outputBase); // Transfer the SPARTA to user
    iBEP20(TOKEN).transfer(member, outputToken); // Transfer the TOKENs to user
    emit RemoveLiquidity(member, outputBase, outputToken, _actualInputUnits);
    return (outputBase, outputToken);
}

Tools Used

Recommended Mitigation Steps

Create a variable curatedPoolCount and increase it in addCuratedPool and decrease it in removeCuratedPool