Salty.IO - 0xOmer's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59

Vulnerability details

Description

ManagedWallet contract is responsible for managing the mainWallet and confirmationWallet.

In a case where we want to changeWallets(), first mainWallet calls proposeWallets() and confirmationWallet sends required ether to establish timelock and after enough time passed proposedMainWallet calls changeWallets() to execute the proposal. changeWallets() implements a code block to reset the proposed wallets to simply make it possible to propose again (to pass the overwrite check).

function changeWallets()
	...
		// Reset
		activeTimelock = type(uint256).max;
		proposedMainWallet = address(0);
		proposedConfirmationWallet = address(0);
		}
	}

When we want to reject a proposal, proposedWallet should just sent less than .05 ether to contract. This only sets activeTimelock to uint256.max and forgets to reset the proposed wallets to be able to proposeWallets again.

    receive() external payable
        {
        require( msg.sender == confirmationWallet, "Invalid sender" );

                // Confirm if .05 or more ether is sent and otherwise reject.
        if ( msg.value >= .05 ether )
                ...
        else
                        activeTimelock = type(uint256).max;
        }

Impact

Since it is impossible to propose again after rejecting a proposal, it must be accepted otherwise changing team wallets would be impossible and given a false address proposed, contract would break.

Proof of Concept

Step-by-step overview a case:

• Dev team wants to change their mainWallet and calls proposeWallets()
• For some reason they want to reject and propose again so they send 1 wei to contract to reject the previous proposal
• They try to call proposeWallets() to propose again but get an error mesage "Cannot overwrite non-zero proposed mainWallet."

Here they have one option, to call changeWallets() with proposedMainWallet if it is in their use or to stick with same wallet addresses forever.

Tools Used

Manual

Recommended Mitigation Steps

Adding required resets inside receive() to pass the checks to call proposeWallets again.

        proposedMainWallet = address(0);
        proposedConfirmationWallet = address(0);

Assessed type

Other

Missing check for wallets to not be the same addresses

ManagedWallet contract introduces 2 wallets mainWallet and confirmationWallet. It is possible to pick them as same addresses. Since they have different roles they must not be same. Consider adding relevant checks both in constructor and proposeWallets() to block the logic fail.

ProposedConfirmationWallet can be overwritten

According to the comment, overwriting a proposal should not allowed. Though the mentioned check exist for proposedMainWallet, there isn't for proposedConfirmationWallet.

approve() should be replaced with safeIncreaseAllowance() / safeDecreaseAllowance()

approve is subject to known front-running attacks and does not handle non-standard ERC20 behaviors. Consider using safeIncreaseAllowance and safeDecreaseAllowance instead:

src/staking/Liquidity.sol:
   86          if (swapAmountA > 0) {
   87:             tokenA.approve(address(pools), swapAmountA);
   88  

  100          else if (swapAmountB > 0) {
  101:             tokenB.approve(address(pools), swapAmountB);
  102  

  154          // Approve the liquidity to add
  155:         tokenA.approve(address(pools), maxAmountA);
  156:         tokenB.approve(address(pools), maxAmountB);
  157  

Keep in mind that safeApprove() is deprecated by OZ and recommended using their safeIncreaseAllowance() and safeDecreaseAllowance()

See this discussion: SafeERC20.safeApprove() Has unnecessary and unsecure added behavior

NOTE: In bot report this issue is mentioned but it recommends to use safeApprove() which is deprecated and not safe to use.

Missing zero address check in PriceAggregator.setPriceFeed()

It should exist to prevent any undesired outcome. Consider adding: require( address(newPriceFeed) != address(0), "newPriceFeed address can not be zero" );