Salty.IO - Ephraim's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L115 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L128

Vulnerability details

Impact

Detailed description of the impact of this finding.

Double burning of USDS tokens occurs when users repay their borrowed USDS.

Users repay their borrowed USDS through the CollateralAndLiquidity.sol. However, these tokens are to be sent into the Liquidizer contract that handles the burning of USDS tokens and holds some USDS token that serves as burnable buffer. When users call the repayUSDS token from the CollateralAndLiquidity, these USDS tokens are directly sent into the usds contract. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125

This is followed with a corresponding increment in the usdsThatShouldBeBurned state variable in the Liquidizer.sol. This variable notes the value of USDS to be burned.

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L128

In the future when upkeep is performed, tokens in the liquidizer would be burned due to the usdsThatShouldBeBurned value and the repaid USDS tokens directly sent into the USDS contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

https://gist.github.com/Ephraim-nonso/420097aa1bf24c5cd722420dce171e2a

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Send repaid tokens to the liquidizer contract instead of usds contract.

-- 		usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

++ 		usds.safeTransferFrom(msg.sender, address(liquidizer), amountRepaid);

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. If the confirmation wallet at any point rejects wallet proposal invoked by the mainWallet, this will deny the mainWallet the ability to propose wallets in the future. The problem is caused by this check:

require(proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet.");

Scenario before a Rejection:

• When mainWallet invokes the proposeWallets function
• proposedMainWallet state variable is updated to the address proposed
• Confirmation wallet rejects with sending less than the required ether

Scenario after a Rejection:

• Reverts when mainWallets invokes proposeWallets function in the future because the proposedMainWallet is non-zero address.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

https://gist.github.com/Ephraim-nonso/be99a50aad9504b0ca34c105e8d109c1

Tools Used

Manual and Foundry

Recommended Mitigation Steps

Update the proposedMainWallet to zero address in the receive function when confirmation wallet sends less than 0.05 ether.

Assessed type

Other