c4-bot-2 commented 4 months ago

Lines of code

https://github.com/othernet-global/salty-io/blob/main/src/dev/Deployment.sol#L213-L214

Vulnerability details

Impact

The protocol's reputation could be damaged due to distributing more SALT than expected to the DAO and development team. It's difficult to pinpoint the direct loss, but at the very least, users' willingness to become liquidity providers on Salty may be affected due to unfair initial SALT distribution.

Proof of Concept

When Salty exchange is actived,

25M SALT will be transferred to daoVestingWallet and 10M SALT will be transferred to teamVestingWallet by calling InitialDistribution#distributionApproved():

56:     // 25 million       DAO Reserve Vesting Wallet
57:     salt.safeTransfer( address(daoVestingWallet), 25 * MILLION_ETHER );
58:
59:     // 10 million       Initial Development Team Vesting Wallet
60:     salt.safeTransfer( address(teamVestingWallet), 10 * MILLION_ETHER );

daoVestingWallet is responsible for distributing 25M SALT to DAO linely over 10 years
teamVestingWallet is responsible for distributing 10M SALT to teamWallet linely over 10 years Check the smart contract deployments in Deployment.sol:
```
213:        daoVestingWallet = new VestingWallet( address(dao), uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );
214:        teamVestingWallet = new VestingWallet( teamWallet, uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );
```
As we can see, the distribution start time of daoVestingWallet and teamVestingWallet is the deployment time. However the exchange is not active at the moment. If we check line 216 in Deployment.sol, we can see that it will take at least 5 days to active the exchange because ballotDuration was initialized to 5 days.
```
    bootstrapBallot = new BootstrapBallot(exchangeConfig, airdrop, 60 * 60 * 24 * 5 );
```
From the above we can see, DAO and teamWallet can get 5 days SALT distribution immediately once the exchanged is active.

Copy below codes to BootstrapBallot.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test test_finalizeBallotThenCheckVestingBalance

    function test_finalizeBallotThenCheckVestingBalance() public {
        // Voting stage (yesVotes: 2, noVotes: 0)
        //@audit-info deploy all contract
        initializeContracts();
        bytes memory sig = abi.encodePacked(aliceVotingSignature);
        vm.startPrank(alice);
        bootstrapBallot.vote(true, sig);
        vm.stopPrank();

        sig = abi.encodePacked(bobVotingSignature);
        vm.startPrank(bob);
        bootstrapBallot.vote(true, sig);
        vm.stopPrank();

        // Increase current blocktime to be greater than completionTimestamp
        vm.warp( bootstrapBallot.completionTimestamp());

        assertEq( salt.balanceOf(address(initialDistribution)), 100000000 ether);

        // Call finalizeBallot()
        bootstrapBallot.finalizeBallot();

        // Verify that the InitialDistribution.distributionApproved() was called.
        assertEq( salt.balanceOf(address(initialDistribution)), 0);
        //@audit-info non-zero SALT can be released to dao and teamWallet immediately 
        assertEq(daoVestingWallet.releasable(address(salt)),   34246575342465753424657);
        assertEq(teamVestingWallet.releasable(address(salt)),  13698630136986301369863);
    }

Tools Used

Manual review

Recommended Mitigation Steps

The vesting start time should not be early than the exchange activation time. It is recommended to deploy daoVestingWallet and teamVestingWallet in InitialDistribution#distributionApproved(), and use block.timestamp as start timestamp.

Assessed type

Context

Picodes commented 4 months ago

Waiting for the sponsor's input but it can make sense to want the team and the DAO to have an initial allocation

c4-sponsor commented 4 months ago

othernet-global (sponsor) confirmed

othernet-global commented 4 months ago

VestingWallets now start at boostrapBallot.completionTimestamp

Fixed in: https://github.com/othernet-global/salty-io/commit/46d395f791a8e3a5d8753eb3f4918cc0e24b23d0

Picodes commented 4 months ago

Following the sponsor's answer, it seems that this was indeed unintended. As a functionality of the protocol isn't working as expected but funds aren't really at risk, I'll validate under Medium severity

c4-judge commented 4 months ago

Picodes marked the issue as satisfactory