Salty.IO - lanrebayode77's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L154 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L104-L111

Vulnerability details

Impact

This vulnerability allows a malicious user to prevent liquidation by frontrunning liquidation transaction with a call to depositCollateralAndIncreaseShare(). This causes the call to liquidateUser() to revert.

This could lead to the protocol having a lot of bad debt.

Proof of Concept

The problem originates from the call within liquidateUser() to _decreaseUserShare() with the coolDown flag set to true. THis call was done to reduce the user share since it's asset are being sold off to pay back loan.

However, with the coolDown being set to true, it gives a malicious actor the opportunity to make the transaction fail by frontrunning it with a call to depositCollateralAndIncreaseShare() to increase shares a with small amount of liquidity, which will extends the user.cooldownExpiration, because of that, liquidateUser() will revert.

if ( useCooldown )
		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
			{
			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

			// Update the cooldown expiration for future transactions
			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
			}

Instance

1. Alice takes a loan of 1_000_000 USDS with the required amount of collateral.
2. Alice collateral value then went below the required amount of value and makes the loan qualified for liquidation.
3. Bob tries to liquidate Alice, but Alice front-run Bob's transaction by adding a very low amount(enough to beat the dust amount) of liquidity to extend user.cooldownExpiration, and Bob transaction reverts.

Currently extension is by an hour, but can be six hours, the longer the extension time, the easier the attack.

Tools Used

Manual review.

Recommended Mitigation Steps

cooldown flag should be set to false in liquiateUser() function.

// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users have the liberty to borrow USDS by providing overcollaterized asset(WBTC/WETH), this asset can with returned back when the user repays the loan, when a user defaults, the user asset are then liquidated and converted to USDS to be burnt.

In the case of liquidation, after another user helps to liquidate an underwater loan for a commission, the asset are sent to Liquidizer contract, when collateralAndLiquidity.liquidizer().performUpkeep(); is called, the liquidizer contract sells the required collaterals that have been sent to convert them to USDS, then calls _possiblyBurnUSDS(); to burn all usdsThatShouldBeBurned

	// This corresponds to USDS that was borrowed by users who had their collateral liquidated.
	// Because liquidated collateral no longer exists, the borrowed USDS needs to be burned in order to "undo" the collateralized position
	// and return state back to where it was before the liquidated user deposited collateral and borrowed USDS.
	uint256 public usdsThatShouldBeBurned;

as stated by the comment above usdsThatShouldBeBurned corresponds to usds that was borrowed by users that had their collateral liquidated.

The problem here is that users that did not have their collateral liquidated also increases the value. This should not be so because in repayUSDS() borrowed USDS have been transferred to USDS contract to be burnt internally

// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
// Have USDS remember that the USDS should be burned
liquidizer.incrementBurnableUSDS( amountRepaid );

The implication will be that, the liquidizer will burn excess USDS than it should

Proof of Concept

Let us consider a case of two borrowers; Alice and Bob

1. Alice and Bob takes a loan of 1_000_000 USDS each with the right amount of shares to keep the loan overcollaterized.
2. Alice calls repayUSDS() to pay back all the debt, 1_000_000 USDS was sent to USDS contract and usdsThatShouldBeBurned was still incremented by same amount. So USDS balance in USDS contract is 1,000,000 and usdsThatShouldBeBurned is 1,000,000

// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
// Have USDS remember that the USDS should be burned
liquidizer.incrementBurnableUSDS( amountRepaid );

3. Bob's loan on the other hand became undercollateralized and due for liquidation.
4. Greg then called the liquidateUser() to liquidate Bob, Bob's collaterals are pulled out and sent to the Liquidizer contract, and usdsThatShouldBeBurned is increased by another 1,000,000 to make it 2,000,000
5. When collateralAndLiquidity.liquidizer().performUpkeep(); was called, required amount of previously sent tokens are swapped for USDS, total from the sales of WETH, WBTC and DAI made USDS balance of the Liquidizer to be 2,000,000.
6. When _possiblyBurnUSDS(); was then called, _burnUSDS( 2,000,000 ) will lead to transferring the 2,000,000 USDS to the USDS contract(Note: this should have been 1,000,000 since the Alice USDS was previously sent to the USDS contract as soon as repayment was done)

	// Burn the specified amount of USDS
	function _burnUSDS(uint256 amountToBurn) internal
		{
		usds.safeTransfer( address(usds), amountToBurn );
		usds.burnTokensInContract();
		}

Now let see what happens in usds.burnTokenInContract()

	// USDS tokens will need to be sent here before they are burned (on a repayment or liquidation).
	// There should otherwise be no USDS balance in this contract.
    function burnTokensInContract() external
    	{
    	uint256 balance = balanceOf( address(this) );
    	_burn( address(this), balance );

    	emit USDSTokensBurned(balance);
    	}
    }

From the code snippet above, we can see that when balance is checked, USDS contract will have 3,000,000 USDS instead of 2,000,000 and all will be burnt, burning excess of 1,000,000 USDS. Meanwhile the excess 1,000,000 USDS should have been left in the Liquidizer contract should incase of when there is shortage of USDS and liquidated amount need to be balanced

		if ( usdsBalance >= usdsThatShouldBeBurned )
			{
			// Burn only up to usdsThatShouldBeBurned.
			// Leftover USDS will be kept in this contract in case it needs to be burned later.
			_burnUSDS( usdsThatShouldBeBurned );
    		usdsThatShouldBeBurned = 0;
			}

Tools Used

Manual review

Recommended Mitigation Steps

// Repay borrowed USDS and adjust the user's usdsBorrowedByUser
function repayUSDS( uint256 amountRepaid ) external nonReentrant
	{
require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
require( amountRepaid > 0, "Cannot repay zero amount" );

// Decrease the borrowed amount for the user
usdsBorrowedByUsers[msg.sender] -= amountRepaid;
// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
	usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

// Have USDS remember that the USDS should be burned
--	liquidizer.incrementBurnableUSDS( amountRepaid ); //@audit remove this
++      usds.burnTokensInContract(); 

// Check if the user no longer has any borrowed USDS
if ( usdsBorrowedByUsers[msg.sender] == 0 )
_walletsWithBorrowedUSDS.remove(msg.sender);

emit RepaidUSDS(msg.sender, amountRepaid);
	}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L103

Vulnerability details

Impact

The creation of proposals can be front-run with another proposal bearing the same name, causing the previous transaction to fail.

Proof of Concept

// Make sure that a proposal of the same name is not already open for the ballot
require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

Tools Used

Manual review.

Recommended Mitigation Steps

Each proposal should have a nonce attached to the name to differentiate them even with the same name.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L417-L439

Vulnerability details

Impact

DAO can propose whitelisting of a token by calling proposeTokenWhitelisting(), with more than one poolsConfig.numberOfWhitelistedPools(), only the token with the highest support vote can be whitelisted at a certain period.

// Fail to whitelist for now if this isn't the whitelisting proposal with the most votes - can try again later.
uint256 bestWhitelistingBallotID = proposals.tokenWhitelistingBallotWithTheMostVotes();
require( bestWhitelistingBallotID == ballotID, "Only the token whitelisting ballot with the most votes can be finalized" );

However, the problem here is that proposals.tokenWhitelistingBallotWithTheMostVotes(); might return an incorrect value under certain condition. For instance when more than one token has the same number of Yes as the highest value of yes.

Proof of Concept

Let us consider a case study;

1. Five tokens were proposed for whitelisting
2. After voting is done; Quorum = 1,000 Token A(BNB) has 600 YES and 400 N0 Token B(PEP) has 300 YES and 700 N0 Token C(AVAX) has 600 YES and 400 N0 Token D(WIN) has 300 YES and 700 N0 Token E(SHB) has 590 YES and 410 NO We can see here that more than one token has the highest number of YES, but when proposals.tokenWhitelistingBallotWithTheMostVotes(); is called, Token A will be returned as the one with the most YES vote, which is not correct in this case because Token C also has 600 YES votes. However, Token A will be whitelisted because of its placement in the array and not because it has the highest YES votes while Token C with the same number of YES votes will be discarded.

		{
		uint256 quorum = requiredQuorumForBallotType( BallotType.WHITELIST_TOKEN);
uint256 bestID = 0;
uint256 mostYes = 0;
for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
{
uint256 ballotID = _openBallotsForTokenWhitelisting.at(i);
uint256 yesTotal = _votesCastForBallot[ballotID][Vote.YES];
uint256 noTotal = _votesCastForBallot[ballotID][Vote.NO];

  if ( (yesTotal + noTotal) >= quorum ) // Make sure that quorum has been reached
  if ( yesTotal > noTotal )  // Make sure the token vote is favorable
@ if ( yesTotal > mostYes )  // Make sure these are the most yes votes seen
{
bestID = ballotID;
mostYes = yesTotal;
  }
}

return bestID;
}

Tools Used

Manual review.

Recommended Mitigation Steps

Modify the code in such a way that, when there is more than one token with the highest number of YES, have a condition to settle for that. This might be to discard the ballot or to find the token with the highest number of total votes(highest participation).

Assessed type

Invalid Validation