Malt Finance contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Gas is overspent on function call and calculations

Proof of Concept

First, save Malt and Collateral tokens decimals difference to storage variable. As neither Malt, nor Collateral token decimals change since initial setup, both can be saved and accessed as a storage variable instead of calling decimals() function and calculating the difference each time.

Second, now sellMalt calls costBasis, which already retrieved decimals and their difference, but sellMalt ignores those, retrieving them from functions/storage again. This could be unified as discussed below.

sellMalt: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/SwingTrader.sol#L77 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/SwingTrader.sol#L109

costBasis: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/SwingTrader.sol#L146

Recommended Mitigation Steps

Save both decimals values to contract storage variables and use them instead of decimals() function. As the calculations use decimals difference it might be enough to save and use the difference only. In any case saving is preferred to calling as the latter spend gas on call and storage access anyway.

Also, return the difference along with decimals from costBasis and use them in sellMalt instead of obtaining afresh. I.e. first reuse costBasis returned decimals instead of collateralToken.decimals(), then add maltDecimals and the difference, whether maltDecimals - decimals or decimals - maltDecimals to its output and use in rewards / soldBasis calculations. Function arguments and returned values are memory and are cheaper than another storage access.

Now:

(uint256 basis,) = costBasis();
...
uint256 maltDecimals = malt.decimals();
uint256 decimals = collateralToken.decimals();
...
uint256 diff = maltDecimals - decimals; 

To be:

(uint256 basis, uint256 decimals, uint256 maltDecimals, uint256 diff) = costBasis();
...

Handle

hyh

Vulnerability details

Impact

Gas is overspent on access and operations.

Proof of Concept

amountOut is calculated in 3 steps, which can be made simpler. https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L302-305

Recommended Mitigation Steps

Now:

uint256 amountTokens = commitment.commitment.mul(auction.pegPrice).div(price);
uint256 redeemedTokens = commitment.redeemed.mul(auction.pegPrice).div(price);
uint256 amountOut = amountTokens.mul(claimablePerc).div(auction.pegPrice).sub(redeemedTokens);

To be (amountTokens and redeemedTokens aren't used elsewhere):

 /*
* uint256 amountTokens = commitment.commitment.mul(auction.pegPrice).div(price);
* uint256 redeemedTokens = commitment.redeemed.mul(auction.pegPrice).div(price);
* uint256 amountOut = amountTokens.mul(claimablePerc).div(auction.pegPrice).sub(redeemedTokens);
*/
uint256 redeemed = commitment.redeemed.mul(auction.pegPrice);
uint256 amountOut = commitment.commitment.mul(claimablePerc).sub(redeemed).div(price);

Handle

hyh

Vulnerability details

Impact

Gas is overspent on access and operations.

Proof of Concept

claimablePerc is calculated in two steps, which can be made simpler. https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L294

Recommended Mitigation Steps

Now:

uint256 totalTokens = auction.commitments.mul(auction.pegPrice).div(auction.finalPrice);
uint256 claimablePerc = auction.claimableTokens.mul(auction.pegPrice).div(totalTokens);

To be (totalTokens isn't used elsewhere):

uint256 claimablePerc = auction.claimableTokens.mul(auction.finalPrice).div(auction.commitments);

Handle

hyh

Vulnerability details

Impact

Gas is overspent on operations.

Proof of Concept

The function contains two non-intersecting logic pathways, which can be separated to lighten calculations.

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AbstractRewardMine.sol#L179

Recommended Mitigation Steps

Now:

function _handleStakePadding(address account, uint256 amount) internal {
	uint256 totalRewardedWithStakePadding = totalDeclaredReward().add(totalStakePadding());

	uint256 INITIAL_STAKE_SHARE_MULTIPLE = 1e6;

	uint256 bondedTotal = totalBonded();

	uint256 newStakePadding = bondedTotal == 0 ?
		totalDeclaredReward() == 0 ? amount.mul(INITIAL_STAKE_SHARE_MULTIPLE) : 0 :
		totalRewardedWithStakePadding.mul(amount).div(bondedTotal);

	_addToStakePadding(account, newStakePadding);
}

To be:

function _handleStakePadding(address account, uint256 amount) internal {
	uint256 bondedTotal = totalBonded();
	
	uint256 newStakePadding;
	if (bondedTotal == 0) {
		uint256 INITIAL_STAKE_SHARE_MULTIPLE = 1e6;
		newStakePadding = totalDeclaredReward() == 0 ? amount.mul(INITIAL_STAKE_SHARE_MULTIPLE) : 0;
	} else {
		newStakePadding = (totalDeclaredReward().add(totalStakePadding())).mul(amount).div(bondedTotal);
	}

	if (newStakePadding > 0)
		_addToStakePadding(account, newStakePadding);
}

Handle

hyh

Vulnerability details

Impact

Gas is overspent on access and function calls.

Proof of Concept

totalDeclaredReward is called by _handleStakePadding twice: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AbstractRewardMine.sol#L180

While totalDeclaredReward does expensive balanceOf call: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AbstractRewardMine.sol#L97

Recommended Mitigation Steps

It is viable to at least remove its double usage:

Now:

uint256 totalRewardedWithStakePadding = totalDeclaredReward().add(totalStakePadding());
...
uint256 newStakePadding = bondedTotal == 0 ?
	totalDeclaredReward() == 0 ? amount.mul(INITIAL_STAKE_SHARE_MULTIPLE) : 0 :
	totalRewardedWithStakePadding.mul(amount).div(bondedTotal);

To be:

uint256 declaredRewardTotal =  rewardToken.balanceOf(address(this));
uint256 totalRewardedWithStakePadding = declaredRewardTotal.add(totalStakePadding());
...
uint256 newStakePadding = bondedTotal == 0 ?
	declaredRewardTotal == 0 ? amount.mul(INITIAL_STAKE_SHARE_MULTIPLE) : 0 : totalRewardedWithStakePadding.mul(amount).div(bondedTotal);

Handle

hyh

Vulnerability details

Impact

Gas is overspent on function calls.

Proof of Concept

earned function calls public getRewardOwnershipFraction function: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AbstractRewardMine.sol#L144

Recommended Mitigation Steps

Now:

function totalDeclaredReward() virtual public view returns (uint256) {
	return rewardToken.balanceOf(address(this));
}
function getRewardOwnershipFraction(address account) public view returns(uint256 numerator, uint256 denominator) {
	numerator = balanceOfRewards(account);
	denominator = totalDeclaredReward();
}
...
function earned(address account) public view returns (uint256 earnedReward) {
	(uint256 rewardNumerator, uint256 rewardDenominator) = getRewardOwnershipFraction(account);

To be:

function earned(address account) public view returns (uint256 earnedReward) {
	uint256 rewardNumerator = balanceOfRewards(account);
	uint256 rewardDenominator = rewardToken.balanceOf(address(this));

Handle

hyh

Vulnerability details

Impact

Gas is overspent on storage access, which happens each time when the variable is mutable.

Proof of Concept

Token variables cannot be changed, but aren't immutable in a number of contracts.

ForfeitHandler https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/ForfeitHandler.sol#L13

RewardReinvestor https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/RewardReinvestor.sol#L22

Auction https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L77

UniswapHandler https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L26

Recommended Mitigation Steps

ForfeitHandler example, the rest are very same.

ERC20 public immutable rewardToken;

Handle

hyh

Vulnerability details

Impact

ERC20 balanceOf call is costly. Malt balance is read twice in sellMalt call, which isn't needed, so gas is overspent here.

Proof of Concept

Malt balanceOf(address(this)) is called twice: https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/SwingTrader.sol#L86 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/SwingTrader.sol#L150

Recommended Mitigation Steps

It's recommended to make internal version of costBasis that takes Malt balance as an argument.

Now:

uint256 totalMaltBalance = malt.balanceOf(address(this));
...
(...) = costBasis();
...
function costBasis() public view returns (uint256 cost, uint256 decimals) {
...
uint256 maltBalance = malt.balanceOf(address(this));
...

To be:

uint256 totalMaltBalance = malt.balanceOf(address(this));
...
(...) = _costBasis(totalMaltBalance);
...
function _costBasis(uint256 maltBalance) internal view returns (...) {
...
function costBasis() public view returns (...) {
	return _costBasis(malt.balanceOf(address(this)));
}