Open code423n4 opened 2 years ago
The maximum fund loss in this case is capped by the maximum number of routers allowed in a transfer (this will generally be below 10, meaning maximum loss from one of these errors is 18 wei units of the token -- assuming it hit max on both execute and reconcile).
I understand the rounding error exists, but even with tokens at a precision of 6 with a high price the maximum rounding error is small (i.e. 100K coin, 6 decimals, this amounts to $1.8). At this level of impact, this should amount to a value leak.
I'm gonna downgrade this to QA
because the value leak is mostly negligible and extremely hard to avoid. You could sweep the remaining balance and delegate that to the last router, but this is unfair to other routers.
I think a good solution would be to make the last router pay the swept balance and then be reconciled this amount once the bridge transfer is complete.
Actually, upon thinking about this more, I think there is potential for the system to not work as intended under certain parts of the transfer flow.
If the bridge transfer amount is 10 DAI
and the liquidity must be provided three routers, each router provides 3 DAI
respectively. If the user opted to receive the local asset than 10 DAI
will be directly sent out to them. Therefore, until the transfer has been reconciled, the protocol will essentially take on temporary bad debt which won't be resolved until the bridge transfer has been reconciled. This may limit withdrawals for other routers during this period. For these reasons, I think medium
severity makes more sense:)
Lines of code
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L753-L803 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L526-L616
Vulnerability details
Impact
variable
routerBalances
suppose to keep track of routers balance in contract androuters
can withdraw their balance from contract. but because of division rounding error in_handleExecuteLiquidity()
and_reconcile()
contract uses more of its tokens than it subtract from router's balance. this can lead to fund lose.Proof of Concept
This is
_handleExecuteLiquidity()
code:As you can see in last part it uses
uint256 routerAmount = toSwap / pathLen
to calculate amount to decrease from router's balance but because of division rounding error contract usingtoSwap
amount of token buy total value it decrease from router's balances are lower thantoSwap
.Of course in
_reconcile()
contract add some amount to router's balances again with division rounding error, but because the amounts are different in this two functions (in_handleExecuteLiquidity()
we subtract fee and in_reconcile()
we don't subtract fee) so the division rounding error would be different for them and in the end sum of total balances of routers would not be equal to contract balance. this bug could be more serious for tokens with low precision and higher price.Tools Used
VIM
Recommended Mitigation Steps
perform better calculations.