c4-bot-4 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/Upkeep.sol#L231-L239

Vulnerability details

Impact

The Development Team could potentially incur a loss on their SALT distribution reward due to the absence of access control on VestingWallet#release().

Proof of Concept

When Salty exchange is actived, 10M SALT will be transferred to teamVestingWallet by calling InitialDistribution#distributionApproved():

62:     salt.safeTransfer( address(teamVestingWallet), 10 * MILLION_ETHER );

teamVestingWallet is responsible for distributing 10M SALT linely over 10 years (Deployment.sol#L100):

    teamVestingWallet = new VestingWallet( address(upkeep), uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );

From the above code we can see that the beneficiary of teamVestingWallet is Upkeep.

Each time Upkeep#performUpkeep() is called, teamVestingWallet will release a certain amount of SALT to Upkeep, the beneficiary, and then the relased SALT will be transferred to mainWallet of managedTeamWallet:

  function step11() public onlySameContract
  {
    uint256 releaseableAmount = VestingWallet(payable(exchangeConfig.teamVestingWallet())).releasable(address(salt));

    // teamVestingWallet actually sends the vested SALT to this contract - which will then need to be sent to the active teamWallet
    VestingWallet(payable(exchangeConfig.teamVestingWallet())).release(address(salt));

    salt.safeTransfer( exchangeConfig.managedTeamWallet().mainWallet(), releaseableAmount );
  }

However, there is no access control on teamVestingWallet.release(). Any one can call release() to distribute SALT without informing upkeep. upkeep doesn't know how many SALT has been distributed in advance, it has no way to transfer it to the development team, and the distributed SALT by directly calling teamVestingWallet.release() will be locked in upkeep forever.

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

  function testTeamRewardIsLockedInUpkeep() public {
    uint releasableAmount = teamVestingWallet.releasable(address(salt));
    uint upKeepBalance = salt.balanceOf(address(upkeep));
    uint mainWalletBalance = salt.balanceOf(address(managedTeamWallet.mainWallet()));
    //@audit-info a certain amount of SALT is releasable
    assertTrue(releasableAmount != 0);
    //@audit-info there is no SALT in upkeep
    assertEq(upKeepBalance, 0);
    //@audit-info there is no SALT in mainWallet
    assertEq(mainWalletBalance, 0);
    //@audit-info call release() before performUpkeep()
    teamVestingWallet.release(address(salt));
    upkeep.performUpkeep();

    upKeepBalance = salt.balanceOf(address(upkeep));
    mainWalletBalance = salt.balanceOf(address(managedTeamWallet.mainWallet()));
    //@audit-info all released SALT is locked in upKeep
    assertEq(upKeepBalance, releasableAmount);
    //@audit-info development team receive nothing
    assertEq(mainWalletBalance, 0);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Since exchangeConfig.managedTeamWallet is immutable, it is reasonable to config managedTeamWallet as the beneficiary when deploying teamVestingWallet:
teamVestingWallet = new VestingWallet( address(upkeep), uint64(block.timestamp), 60 60 24 365 10 );
teamVestingWallet = new VestingWallet( address(managedTeamWallet), uint64(block.timestamp), 60 60 24 365 10 );

Introduce a new function in managedTeamWallet to transfer all SALT balance to mainWallet:

function release(address token) external {
uint balance = IERC20(token).balanceOf(address(this));
if (balance != 0) {
  IERC20(token).safeTransfer(mainWallet, balance);
}
}

call managedTeamWallet#release() in Upkeep#performUpkeep():
```
function step11() public onlySameContract
{
```
uint256 releaseableAmount = VestingWallet(payable(exchangeConfig.teamVestingWallet())).releasable(address(salt));
// teamVestingWallet actually sends the vested SALT to this contract - which will then need to be sent to the active teamWallet VestingWallet(payable(exchangeConfig.teamVestingWallet())).release(address(salt));
salt.safeTransfer( exchangeConfig.managedTeamWallet().mainWallet(), releaseableAmount );
exchangeConfig.managedTeamWallet().release(address(salt)); }

Assessed type

Other

c4-judge commented 9 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 9 months ago

othernet-global (sponsor) confirmed

othernet-global commented 8 months ago

The ManagedWallet now the recipient of teamVestingWalletRewards to prevent the issue of DOS of the team rewards.

https://github.com/othernet-global/salty-io/commit/534d04a40c9b5821ad4e196095df70c0021d15ab

othernet-global commented 8 months ago

ManagedWallet has been removed.

https://github.com/othernet-global/salty-io/commit/5766592880737a5e682bb694a3a79e12926d48a5

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

0xRobocop commented 8 months ago

I think this issue was miss-judged.

The issue was found in code that was not in scope:

teamVestingWallet is not in the list of contracts in scope.
Deployment.sol (where the root cause is) is explicitly written as out of scope.

etherSky111 commented 8 months ago

Can be treated as medium.

From C4 doc: "Fault in out-of-scope library with impact on an in-scope contract When an in-scope contract composes/inherits with an OOS contract, and the root cause exists in the OOS contract, the finding is to be treated as OOS. Exceptional scenarios are at the discretion of the judge."

From Immunefi: "Impacts caused by exploiting external dependencies

Critical & High severity impacts caused by exploiting external dependencies (such as Chainlink oracles and OpenZepplin libraries) are considered valid and in-scope, however they will be downgraded to Medium severity from the assessed impact."

piken commented 8 months ago

The root cause is that performUpkeep() send the wrong amount of SALT to the main wallet of the manage team in step11(). The reason to change Deployment.sol is that it will be the simplest way to fix the issue. But it can also be fixed in step11() by transferring the right amount of SALT to the main wallet of the manage team without updating the deployment code.

No matter which remediation is used, it doesn't change the fact that the root cause is sending the wrong amount of SALT to the main wallet of the manage team in Upkeep#step11().

Picodes commented 8 months ago

My initial view on this is that the issue is within Upkeep as it integrates poorly with the vesting wallet. It forgets that there is no access control, so I tend to see this as in scope.