code-423n4 / 2022-04-backd-findings

6 stars 4 forks source link

Staker is overcharged for gas/Keeper is reimbursed too much gas #195

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L550 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L714

Vulnerability details

Impact

Solidity's builtin function gasleft() is used to estimate that amount of gas the Keeper needs to provide to top-up a loan position. However, gasleft() does not take the concept of gas refunds into account.

Two EVM opcodes can refund gas: SELFDESTRUCT and SSTORE. The SSTORE opcode adds 15000 gas to a refund counter whenever a storage location is set to zero.

TopUpAction.execute can, indirectly, set storage locations to zero. Thus the function will sometimes overestimate the amount of gas consumed by the transaction because it does not account for gas refunds. Thus Stakers will have too much gas transferred out of their gas bank and Keepers will be reimbursed too much.

More details can be found in this Stack Exchange answer

Proof of Concept

The tests in the contest repository contain the comment "FIXME: this should succeed but for some reason does not".

Uncommenting the assert statement in this test serves as a PoC. The reason the estimated gas is greater than the actual gas used (tx.gas_used) is that gas refunds occur due to state locations being deleted/set to zero.

Tools Used

Manual inspection

Recommended Mitigation Steps

It will be necessary to detect and take into account gas refunds that occur due to storage locations being zeroed.

However, in the long run, no mitigation will be necessary if EIP-3298 goes through which proposes that gas refunds are removed from Ethereum.

gzeoneth commented 2 years ago

Doesn't impact protocol functionality and cannot actively exploit, considering this as Low / QA. Duplicate of warden's QA report #209