Omniscia AmpleSense Audit

AmplesenseVault Manual Review Findings

AmplesenseVault Manual Review Findings

AVT-01M: Incorrect Withdrawal Argument

Description:

The claim function passes in the totalStaked values instead of the totalStakedFor evaluation for a particular user, causing the function to be inoperable if there are more than one stakes within the system.

Example:

contracts/AmplesenseVault.sol
355function claim() external {
356    (uint256 eth, uint256 token) = getReward(msg.sender);
357    rewards_eth.withdrawFrom(msg.sender, rewards_eth.totalStaked());
358    rewards_eefi.withdrawFrom(msg.sender, rewards_eefi.totalStaked());
359    emit Claimed(msg.sender, eth, token);
360}

Recommendation:

We advise the proper argument to be passed in to the withdrawFrom invocations to ensure proper execution of the code.

Alleviation:

Both invocations now properly pass the totalStakedFor function's result to the reward calls.

AVT-02M: Inexistent Validation of Deposit Amount

Description:

The depositFor function does not validate the amount deposited and allows a user to perform a large number of zero-amount deposits to a particular user at little cost acting as a denial-of-service (DoS) attack.

Example:

contracts/AmplesenseVault.sol
183/**
184    @dev Deposits AMPL into the contract for a specific address
185    @param account Account on which to credit the deposit
186    @param amount Amount of AMPL to take from the user
187*/
188function depositFor(address account, uint256 amount) public {
189    _ampl_token.safeTransferFrom(msg.sender, address(this), amount);
190    _deposits[account].push(DepositChunk(amount, block.timestamp));
191

192    uint256 to_mint = amount / EEFI_DEPOSIT_RATE;
193    uint256 deposit_fee = to_mint.mul(DEPOSIT_FEE_10000).divDown(10000);
194    // send some EEFI to pioneer vault 2 (kMPL stakers) upon initial mint 
195    if(last_positive + MINTING_DECAY > block.timestamp) { // if 90 days without positive rebase do not mint EEFI
196        eefi_token.mint(address(this), deposit_fee);
197        eefi_token.increaseAllowance(pioneer_vault2.staking_contract_token(), deposit_fee);
198        pioneer_vault2.distribute(deposit_fee);
199        eefi_token.mint(msg.sender, to_mint.sub(deposit_fee));
200    }
201    
202    // stake the shares also in the rewards pool
203    rewards_eefi.stakeFor(account, amount);
204    rewards_eth.stakeFor(account, amount);
205    emit Deposit(account, amount, _deposits[account].length);
206    emit StakeChanged(rewards_eth.totalStaked(), block.timestamp);
207}

Recommendation:

We advise a require check to be introduced ensuring that the amount is non-zero as zero-value deposits prevent a user from gradually iterating through malicious deposits by specifying a small amount of AMPL to withdraw in the withdraw prefixed functions.

Alleviation:

The depositFor function was completely removed as a capability rendering this exhibit null.

AVT-03M: Test Code

Description:

The linked code needs to be omitted prior to launch as it allows arbitrary mints.

Example:

contracts/AmplesenseVault.sol
92function TESTMINT(uint256 amount, address who) external onlyOwner() {
93    eefi_token.mint(who, amount);
94}

Recommendation:

We advise the code to be properly omitted in the next iteration of the codebase.

Alleviation:

The linked test code has been properly removed from the codebase.

AVT-04M: Deprecated Native Asset Transfer

Description:

The transfer member accessible in payable variables forwards a fixed gas stipend for the operation which can change on upcoming EIPs as evidenced by f.e. EIP-2929.

Example:

contracts/AmplesenseVault.sol
315// distribute the remainder of purchased ETH (5%) to the DAO treasury
316treasury.transfer(address(this).balance);

Recommendation:

We strongly recommend a more robust payout method to be utilized such as the sendValue function of OpenZeppelin's Address library that forwards the currently available gas and is guaranteed to always be executable.

Alleviation:

The sendValue method of the OpenZeppelin Address library is now properly utilized.

AVT-05M: Dynamic Balance Evaluation

Description:

The contract relies on a dynamic balance evaluation of the AMPL token. Given that these evaluations are directly used to calculate i.e. EEFI mint proportions, it is possible for users to manipulate this value by sending AMPL directly to the contract prior to a rebase operation and thus causing rewards to be minted for AMPL tokens that were never properly staked. Additionally, given that the withdrawals are also evaluated proportionately it would be possible for a user to stake a small amount of AMPL tokens and directly transmit AMPL tokens prior to the lock's expiry to acquire a disproportionate EEFI reward and then withdraw their stake retrieving a portion of the directly transmitted funds back immediately.

Example:

contracts/AmplesenseVault.sol
272function _rebase(uint256 old_supply, uint256 new_supply) internal override {
273        uint256 new_balance = _ampl_token.balanceOf(address(this));
274

275        if(new_supply > old_supply) {
276            // This is a positive AMPL rebase and initates trading and distribuition of AMPL according to parameters (see parameters definitions)
277            last_positive = block.timestamp;
278            require(address(trader) != address(0), "AmplesenseVault: trader not set");
279

280            uint256 changeRatio18Digits = old_supply.mul(10**18).divDown(new_supply);
281            uint256 surplus = new_balance.sub(new_balance.mul(changeRatio18Digits).divDown(10**18));
282

283            uint256 for_eefi = surplus.mul(TRADE_POSITIVE_EEFI_100).divDown(100);
284            uint256 for_eth = surplus.mul(TRADE_POSITIVE_ETH_100).divDown(100);
285            uint256 for_pioneer1 = surplus.mul(TRADE_POSITIVE_PIONEER1_100).divDown(100);
286

287            // 30% ampl remains in vault after positive rebase
288            // use rebased AMPL to buy and burn eefi
289            
290            _ampl_token.approve(address(trader), for_eefi.add(for_eth));
291

292            trader.sellAMPLForEEFI(for_eefi);
293           // 10% of purchased EEFI is sent to the DAO Treasury. The remaining 90% is burned. 
294            uint256 balance = eefi_token.balanceOf(address(this));
295            IERC20(address(eefi_token)).safeTransfer(treasury, balance.mul(TREASURY_EEFI_100).divDown(100));
296            uint256 to_burn = eefi_token.balanceOf(address(this));
297            eefi_token.burn(address(this), to_burn);
298            emit Burn(to_burn);
299            // buy eth and distribute to vaults
300            trader.sellAMPLForEth(for_eth);
301 
302            uint256 to_rewards = address(this).balance.mul(TRADE_POSITIVE_REWARDS_100).divDown(100);
303            uint256 to_pioneer2 = address(this).balance.mul(TRADE_POSITIVE_PIONEER2_100).divDown(100);
304            uint256 to_pioneer3 = address(this).balance.mul(TRADE_POSITIVE_PIONEER3_100).divDown(100);
305            uint256 to_lp_staking = address(this).balance.mul(TRADE_POSITIVE_LPSTAKING_100).divDown(100);
306            rewards_eth.distribute{value: to_rewards}(to_rewards, address(this));
307            pioneer_vault2.distribute_eth{value: to_pioneer2}();
308            pioneer_vault3.distribute_eth{value: to_pioneer3}();
309            staking_pool.distribute_eth{value: to_lp_staking}();
310

311            // distribute ampl to pioneer 1
312            _ampl_token.approve(address(pioneer_vault1), for_pioneer1);
313            pioneer_vault1.distribute(for_pioneer1);
314

315            // distribute the remainder of purchased ETH (5%) to the DAO treasury
316            treasury.transfer(address(this).balance);
317        } else {
318            // If AMPL supply is negative (lower) or equal (at eqilibrium/neutral), distribute EEFI rewards as follows; only if the minting_decay condition is not triggered
319            if(last_positive + MINTING_DECAY > block.timestamp) { //if 90 days without positive rebase do not mint
320                uint256 to_mint = new_balance.divDown(new_supply < last_ampl_supply ? EEFI_NEGATIVE_REBASE_RATE : EEFI_EQULIBRIUM_REBASE_RATE);
321                eefi_token.mint(address(this), to_mint);
322

323/* 
324EEFI Reward Distribution Overview: 
325

326- Trade Positive Rewards_100: Upon neutral/negative rebase, send 45% of EEFI rewards to users staking AMPL in vault 
327- Trade Positive Pioneer2_100: Upon neutral/negative rebase, send 10% of EEFI rewards to users staking kMPL in Pioneer Vault II (kMPL Stakers)
328- Trade Positive Pioneer3_100: Upon neutral/negative rebase, send 5% of EEFI rewards to users staking in Pioneer Vault III (kMPL/ETH LP Token Stakers) 
329- Trade Positive LP Staking_100: Upon neutral/negative rebase, send 35% of EEFI rewards to uses staking LP tokens (EEFI/ETH) 
330*/
331

332

333                uint256 to_rewards = to_mint.mul(TRADE_POSITIVE_REWARDS_100).divDown(100);
334                uint256 to_pioneer2 = to_mint.mul(TRADE_POSITIVE_PIONEER2_100).divDown(100);
335                uint256 to_pioneer3 = to_mint.mul(TRADE_POSITIVE_PIONEER3_100).divDown(100);
336                uint256 to_lp_staking = to_mint.mul(TRADE_POSITIVE_LPSTAKING_100).divDown(100);
337

338                eefi_token.increaseAllowance(address(rewards_eefi), to_rewards);
339                eefi_token.increaseAllowance(address(pioneer_vault2.staking_contract_token()), to_pioneer2);
340                eefi_token.increaseAllowance(address(pioneer_vault3.staking_contract_token()), to_pioneer3);
341                eefi_token.increaseAllowance(address(staking_pool.staking_contract_token()), to_lp_staking);
342                rewards_eefi.distribute(to_rewards, address(this));
343                pioneer_vault2.distribute(to_pioneer2);
344                pioneer_vault3.distribute(to_pioneer3);
345                staking_pool.distribute(to_lp_staking);
346

347                // distribute the remainder (5%) of EEFI to the treasury
348                require(eefi_token.transfer(treasury, eefi_token.balanceOf(address(this))), "AmplesenseVault: Treasury transfer failed");
349            }
350        }
351

352        eefi_token.mint(msg.sender, REBASE_REWARD);
353    }

Recommendation:

We advise this particular characteristic of the system to be closely evaluated and potentially revised by extrapolating and interpolating the tracked AMPL balance of the contract on each rebase depending on whether the supply increased or decreased respectively.

Alleviation:

The Amplesense considered this exhibit and implemented administrative functionality for rebase operations thereby mitigating the attack surface to only the administrator of the system which is considered negligible.

AVT-06M: Inexistent Slippage Arguments

TypeSeverityLocation
Logical FaultMediumAmplesenseVault.sol:L214, L245

Description:

Given that the contract operates with a rebasing currency, the payouts for a particular unstake operation should be accompanied by a "slippage" argument that ensures a minimum of AMPL / maximum of stake units is withdrawn / consumed depending on the withdrawal operation type.

Example:

contracts/AmplesenseVault.sol
209/**
210    @dev Withdraw an amount of AMPL from vault 
211    Shares are auto computed
212    @param amount Amount of AMPL to withdraw
213*/
214function withdrawAMPL(uint256 amount) external {
215    uint256 shares = amount.mul(rewards_eefi.totalStaked()).divDown(_ampl_token.balanceOf(address(this)));
216    require(shares <= totalStakedFor(msg.sender), "AmplesenseVault: Not enough balance");
217    uint256 to_withdraw = shares;
218    // make sure the assets aren't time locked
219    while(to_withdraw > 0) {
220        // either liquidate the deposit, or reduce it
221        DepositChunk storage deposit = _deposits[msg.sender][0];
222        require(deposit.timestamp < block.timestamp.sub(LOCK_TIME), "AmplesenseVault: No unlocked deposits found");
223        if(deposit.amount > to_withdraw) {
224            deposit.amount = deposit.amount.sub(to_withdraw);
225            to_withdraw = 0;
226        } else {
227            to_withdraw = to_withdraw.sub(deposit.amount);
228            _popDeposit();
229        }
230    }
231    _ampl_token.safeTransfer(msg.sender, amount);
232    
233    // unstake the shares also from the rewards pool
234    rewards_eefi.unstakeFrom(msg.sender, shares);
235    rewards_eth.unstakeFrom(msg.sender, shares);
236    emit Withdrawal(msg.sender, amount,_deposits[msg.sender].length);
237    emit StakeChanged(rewards_eth.totalStaked(), block.timestamp);
238}
239

240/**
241    @dev Withdraw an amount of shares
242    @param amount Amount of shares to withdraw
243    !!! This isnt the amount of AMPL the user will get because the amount of AMPL provided depends on the rebase and distribution of rebased AMPL during positive AMPL rebases
244*/
245function withdraw(uint256 amount) public {
246    require(amount <= totalStakedFor(msg.sender), "AmplesenseVault: Not enough balance");
247    uint256 to_withdraw = amount;
248    // make sure the assets aren't time locked - all AMPL deposits into are locked for 90 days and withdrawal request will fail if timestamp of deposit < 90 days
249    while(to_withdraw > 0) {
250        // either liquidate the deposit, or reduce it
251        DepositChunk storage deposit = _deposits[msg.sender][0];
252        require(deposit.timestamp < block.timestamp.sub(LOCK_TIME), "AmplesenseVault: No unlocked deposits found");
253        if(deposit.amount > to_withdraw) {
254            deposit.amount = deposit.amount.sub(to_withdraw);
255            to_withdraw = 0;
256        } else {
257            to_withdraw = to_withdraw.sub(deposit.amount);
258            _popDeposit();
259        }
260    }
261    // compute the current ampl count representing user shares
262    uint256 ampl_amount = _ampl_token.balanceOf(address(this)).mul(amount).divDown(rewards_eefi.totalStaked());
263    _ampl_token.safeTransfer(msg.sender, ampl_amount);
264    
265    // unstake the shares also from the rewards pool
266    rewards_eefi.unstakeFrom(msg.sender, amount);
267    rewards_eth.unstakeFrom(msg.sender, amount);
268    emit Withdrawal(msg.sender, ampl_amount,_deposits[msg.sender].length);
269    emit StakeChanged(rewards_eth.totalStaked(), block.timestamp);
270}

Recommendation:

We strongly recommend an additional argument to be introduced to both withdrawAMPL and withdraw to allow users to specify such slippage style arguments and ensure expected behaviour by the contract system in case of on-chain integration.

Alleviation:

This exhibit was partially alleviated by introducing a slippage check for the withdrawAMPL operation and not for the withdraw function.

AVT-07M: Potential Denial of Service Attack

Description:

The depositFor function allows a user to make arbitrary deposits to another and the staking mechanism of the vault contains a sorted array that is shifted each time an element from it is omitted. As a result, a user would be able to create multiple dust amount deposits for a user that would cause them to be unable or spend a significant amount of money to withdraw their stake afterwards due to reaching the block gas limit.

Example:

contracts/AmplesenseVault.sol
209/**
210    @dev Withdraw an amount of AMPL from vault 
211    Shares are auto computed
212    @param amount Amount of AMPL to withdraw
213*/
214function withdrawAMPL(uint256 amount) external {
215    uint256 shares = amount.mul(rewards_eefi.totalStaked()).divDown(_ampl_token.balanceOf(address(this)));
216    require(shares <= totalStakedFor(msg.sender), "AmplesenseVault: Not enough balance");
217    uint256 to_withdraw = shares;
218    // make sure the assets aren't time locked
219    while(to_withdraw > 0) {
220        // either liquidate the deposit, or reduce it
221        DepositChunk storage deposit = _deposits[msg.sender][0];
222        require(deposit.timestamp < block.timestamp.sub(LOCK_TIME), "AmplesenseVault: No unlocked deposits found");
223        if(deposit.amount > to_withdraw) {
224            deposit.amount = deposit.amount.sub(to_withdraw);
225            to_withdraw = 0;
226        } else {
227            to_withdraw = to_withdraw.sub(deposit.amount);
228            _popDeposit();
229        }
230    }
231    _ampl_token.safeTransfer(msg.sender, amount);
232    
233    // unstake the shares also from the rewards pool
234    rewards_eefi.unstakeFrom(msg.sender, shares);
235    rewards_eth.unstakeFrom(msg.sender, shares);
236    emit Withdrawal(msg.sender, amount,_deposits[msg.sender].length);
237    emit StakeChanged(rewards_eth.totalStaked(), block.timestamp);
238}

Recommendation:

We strongly recommend the deposit management mechanism to be revised by retaining a sorted linked list structure using mapping declarations rather than arrays significantly optimizing the codebase's gas cost and usability.

Alleviation:

The depositFor function was completely removed as a capability rendering this exhibit null.

Prev

Pioneer1Vault.sol (PVT-S)

Next

BalancerTrader.sol (BTR-M)

On this page